[PATCH 1/6] tegra: fdt: Add keyboard controller definition

[PATCH 1/6] tegra: fdt: Add keyboard controller definition

[Simon Glass]

From: Anton Staff <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

The Tegra keyboard controller provides a simple interface to a matrix
keyboard.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 arch/arm/dts/tegra20.dtsi |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index d7ab843..5344b39 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -234,5 +234,9 @@
 		periph-id = <59>;	// PERIPH_ID_USB3
 	};
 
+	kbc@7000e200 {
+		compatible = "nvidia,tegra20-kbc";
+		reg = <0x7000e200 0x0078>;
+	};
 };
 
-- 
1.7.3.1

[PATCH 2/6] tegra: fdt: Add keyboard definitions for Seaboard

[Simon Glass]

From: Anton Staff <robotboy@chromium.org>

Seaboard uses a QUERTY keyboard. We add key codes for this to
enable key scanning to work.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/nvidia/dts/tegra2-seaboard.dts |   71 ++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
index 19c6503..79de16b 100644
--- a/board/nvidia/dts/tegra2-seaboard.dts
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -45,4 +45,75 @@
 	usb@c5008000 {
 		status = "okay";
 	};
+
+	kbc@7000e200 {
+		status = "okay";
+		keycode-plain = [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  00  00  00
+				 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];
+
+		keycode-shift = [00  00  'W' 'S' 'A' 'Z' 00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 '%' '$' 'R' 'E' 'F' 'D' 'X' 00
+				 '&' '^' 'T' 'H' 'G' 'V' 'C' ' '
+				 '(' '*' 'U' 'Y' 'J' 'N' 'B' '|'
+				 '_' ')' 'O' 'I' 'L' 'K' '<' 'M'
+				 00  '+' '}' 0D  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 '{' 'P' '"' ':' '?' '>' 00  00
+				 00  00  08  '#' '@' 00  00  00
+				 00  7F  00  00  00  00  00  00
+				 00  00  00  'Q' 00  00  '!' 00
+				 1B  '~' 00  09  00  00  00  00];
+
+		keycode-fn =    [00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 '7' 00  00  00  00  00  00  00
+				 '9' '8' '4' 00  '1' 00  00  00
+				 00  '/' '6' '5' '3' '2' 00  '0'
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  27  00  '-' '+' '.' 00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  '?' 00  00  00];
+
+		keycode-ctrl =  [00  00  17  13  01  1a  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  12  05  06  04  18  00
+				 00  00  14  08  07  16  03  00
+				 00  00  15  19  0a  0e  02  00
+				 00  00  0f  09  0c  0b  00  0d
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  10  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  00  00  00  00  00
+				 00  00  00  11  00  00  00  00
+				 00  00  00  00  00  00  00  00];
+	};
 };
-- 
1.7.3.1

[PATCH 3/6] fdt: Add fdtdec functions to read byte array

[Simon Glass]

From: Anton Staff <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Sometimes we don't need a full cell for each value. This provides
a simple function to read a byte array, both with and without
copying it.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 include/fdtdec.h |   32 ++++++++++++++++++++++++++++++++
 lib/fdtdec.c     |   24 ++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 9871b81..f72d219 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -231,3 +231,35 @@ int fdtdec_decode_gpio(const void *blob, int node, const char *prop_name,
  * @return 0 if all ok or gpio was FDT_GPIO_NONE; -1 on error
  */
 int fdtdec_setup_gpio(struct fdt_gpio_state *gpio);
+
+/**
+ * Look up a property in a node and return its contents in a byte
+ * array of given length. The property must have at least enough data for
+ * the array (count bytes). It may have more, but this will be ignored.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @param array		array to fill with data
+ * @param count		number of array elements
+ * @return 0 if ok, or -FDT_ERR_MISSING if the property is not found,
+ *		or -FDT_ERR_BADLAYOUT if not enough data
+ */
+int fdtdec_get_byte_array(const void *blob, int node, const char *prop_name,
+		u8 *array, int count);
+
+/**
+ * Look up a property in a node and return a pointer to its contents as a
+ * byte array of given length. The property must have at least enough data
+ * for the array (count bytes). It may have more, but this will be ignored.
+ * The data is not copied.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @param count		number of array elements
+ * @return pointer to byte array if found, or NULL if the property is not
+ *		found or there is not enough data
+ */
+const u8 *fdtdec_locate_byte_array(const void *blob, int node,
+			     const char *prop_name, int count);
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 509b81f..90db53c 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -315,3 +315,27 @@ int fdtdec_setup_gpio(struct fdt_gpio_state *gpio)
 		return gpio_direction_input(gpio->gpio);
 	}
 }
+
+int fdtdec_get_byte_array(const void *blob, int node, const char *prop_name,
+		u8 *array, int count)
+{
+	const u8 *cell;
+	int err;
+
+	cell = get_prop_len(blob, node, prop_name, count, &err);
+	if (!err)
+		memcpy(array, cell, count);
+	return err;
+}
+
+const u8 *fdtdec_locate_byte_array(const void *blob, int node,
+			     const char *prop_name, int count)
+{
+	const u8 *cell;
+	int err;
+
+	cell = get_prop_len(blob, node, prop_name, count, &err);
+	if (err)
+		return NULL;
+	return cell;
+}
-- 
1.7.3.1

Re: [PATCH 2/6] tegra: fdt: Add keyboard definitions for Seaboard

[Stephen Warren]

On 12/02/2011 07:57 PM, Simon Glass wrote:
> From: Anton Staff <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Seaboard uses a QUERTY keyboard. We add key codes for this to
> enable key scanning to work.

> +++ b/board/nvidia/dts/tegra2-seaboard.dts
...
> +	kbc@7000e200 {
> +		status = "okay";

That's the default, so this isn't needed; instead, add status =
"disabled" to any boards that don't use kbc.

-- 
nvpublic

Re: [PATCH 3/6] fdt: Add fdtdec functions to read byte array

[Stephen Warren]

On 12/02/2011 07:57 PM, Simon Glass wrote:
> From: Anton Staff <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Sometimes we don't need a full cell for each value. This provides
> a simple function to read a byte array, both with and without
> copying it.

> +const u8 *fdtdec_locate_byte_array(const void *blob, int node,
> +			     const char *prop_name, int count)
> +{
> +	const u8 *cell;
> +	int err;
> +
> +	cell = get_prop_len(blob, node, prop_name, count, &err);

Isn't that called get_prop_check_min_len() now?

> +	if (err)
> +		return NULL;
> +	return cell;
> +}

-- 
nvpublic

Re: [PATCH 3/6] fdt: Add fdtdec functions to read byte array

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 3:54 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 07:57 PM, Simon Glass wrote:
>> From: Anton Staff <robotboy@chromium.org>
>>
>> Sometimes we don't need a full cell for each value. This provides
>> a simple function to read a byte array, both with and without
>> copying it.
>
>> +const u8 *fdtdec_locate_byte_array(const void *blob, int node,
>> +                          const char *prop_name, int count)
>> +{
>> +     const u8 *cell;
>> +     int err;
>> +
>> +     cell = get_prop_len(blob, node, prop_name, count, &err);
>
> Isn't that called get_prop_check_min_len() now?

It depends on your definition of 'now'. I will of course rebase this
series and test once the usb one is ready. At present it is probably
based on the original v1 usb series. But there may be other changes
too...

Regards,
Simon

>
>> +     if (err)
>> +             return NULL;
>> +     return cell;
>> +}
>
> --
> nvpublic

Re: [PATCH 4/6] tegra: Add tegra keyboard support

[Stephen Warren]

On 12/02/2011 07:57 PM, Simon Glass wrote:
> Add support for internal matrix keyboard controller for Nvidia Tegra platforms.
> This driver uses the fdt decode function to obtain its key codes.

> +static int fdt_decode_kbc(const void *blob, int node, struct keyb *config)
> +{
> +       config->kbc = (struct kbc_tegra *)fdtdec_get_addr(blob, node, "reg");
> +       config->plain_keycode = fdtdec_locate_byte_array(blob, node,
> +                               "keycode-plain", KBC_KEY_COUNT);
> +       config->shift_keycode = fdtdec_locate_byte_array(blob, node,
> +                               "keycode-shift", KBC_KEY_COUNT);
> +       config->fn_keycode = fdtdec_locate_byte_array(blob, node,
> +                               "keycode-fn", KBC_KEY_COUNT);
> +       config->ctrl_keycode = fdtdec_locate_byte_array(blob, node,
> +                               "keycode-ctrl", KBC_KEY_COUNT);
> +       if (!config->plain_keycode) {
> +               debug("%s: cannot find keycode-plain map\n", __func__);
> +               return -1;
> +       }
> +       return 0;
> +}

Simon,

Sorry to keep pushing back on everything so much, but I don't believe
the structure of this binding is correct.

>From a purely HW perspective, the only thing that exists is a single
mapping from (row, col) to keycode. I believe that's all the KBC
driver's binding should define.

I'll amend that slightly: keyboards commonly implement the Fn key
internally in HW, since it causes keys to emit completely different raw
codes, so I can see this driver wanting both keycode-plain and keycode-fn.

However, keycode-shift and keycode-ctrl are purely SW concepts. As such,
they shouldn't be part of the KBC's own mapping table. Witness how the
kernel's Tegra KBC driver only contains the plain and fn tables (albeit
merged into a single array for ease of use), and the input core is what
interpets e.g. KEY_LEFTSHIFT as a modifier.

So specifically what I'd like to see changed in this binding is:

a) We need binding documentation for the Tegra KBC binding, in the same
style as found in the kernel's Documentation/devicetree/bindings.

b) The Tegra KBC binding should include only the keycode-plain and
keycode-fn properties; nothing to do with shift/ctrl/alt/.... (Well,
also any standardized properties such as compatible, reg, interrupts).

c) We need binding documentation for the data that is/could-be common
across multiple keyboards: i.e. what does each key code value represent;
which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common
across (1) all keyboards (2) some standardized meaning for DT that can
be used by U-Boot, the Linux kernel, etc. Perhaps there is already such
a standard?

d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a
separate binding that can be common to any/all keyboards (probably the
same document as (b) above). The input to this table should be the raw
codes from keycode-plain/keycode-fn. The output would be the values sent
to whatever consumes keyboard input. The naming of these properties
should make it obvious that it's something not specific to Tegra's KBC,
and SW oriented. Perhaps something semantically similar to loadkeys' or
xkbcomp's input, although I haven't looked at those in detail.

Linux probably wouldn't use (d), since it already has its own
SW-oriented methods of setting up such mapping tables. Although perhaps
in the future the default mappings could be set up from these properties.

U-Boot would need (d), since AIUI, there is no handling of such a higher
layer of mapping tables there right now.

Initially, the code to parse and implement the mappings in (d) could be
part of the Tegra KBC driver, if there's nowhere else to put it. I
simply want to ensure that the structure of the mapping table bindings
doesn't require them to be specific to Tegra KBC, or perpetually
implemented by the Tegra KBC driver even when/if U-Boot does acquire a
higher layer that deals with this kind of thing. That's because they're
SW concepts not part of the HW/HW-binding.

-- 
nvpublic

Re: [PATCH 4/6] tegra: Add tegra keyboard support

[Simon Glass]

Hi Stephen,

On Wed, Dec 7, 2011 at 2:02 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 12/02/2011 07:57 PM, Simon Glass wrote:
>> Add support for internal matrix keyboard controller for Nvidia Tegra platforms.
>> This driver uses the fdt decode function to obtain its key codes.
>
>> +static int fdt_decode_kbc(const void *blob, int node, struct keyb *config)
>> +{
>> +       config->kbc = (struct kbc_tegra *)fdtdec_get_addr(blob, node, "reg");
>> +       config->plain_keycode = fdtdec_locate_byte_array(blob, node,
>> +                               "keycode-plain", KBC_KEY_COUNT);
>> +       config->shift_keycode = fdtdec_locate_byte_array(blob, node,
>> +                               "keycode-shift", KBC_KEY_COUNT);
>> +       config->fn_keycode = fdtdec_locate_byte_array(blob, node,
>> +                               "keycode-fn", KBC_KEY_COUNT);
>> +       config->ctrl_keycode = fdtdec_locate_byte_array(blob, node,
>> +                               "keycode-ctrl", KBC_KEY_COUNT);
>> +       if (!config->plain_keycode) {
>> +               debug("%s: cannot find keycode-plain map\n", __func__);
>> +               return -1;
>> +       }
>> +       return 0;
>> +}
>
> Simon,
>
> Sorry to keep pushing back on everything so much, but I don't believe
> the structure of this binding is correct.

If I didn't want your feedback I would not have copied you on the
patches :-) It is very useful to know how things are being done in the
kernel and we need to try to keep U-Boot and the kernel as close as
possible on the fdt side (although perhaps not in any other way -
U-Boot is a boot loader not an OS). I'm starting to form the view that
U-Boot will need a few more tweaks on top of the kernel file in some
cases, but let's see. Thanks very much for taking the time to provide
this very useful feedback. If nothing else these discussions might
tease out issues to address later.

>
> From a purely HW perspective, the only thing that exists is a single
> mapping from (row, col) to keycode. I believe that's all the KBC
> driver's binding should define.
>
> I'll amend that slightly: keyboards commonly implement the Fn key
> internally in HW, since it causes keys to emit completely different raw
> codes, so I can see this driver wanting both keycode-plain and keycode-fn.
>
> However, keycode-shift and keycode-ctrl are purely SW concepts. As such,
> they shouldn't be part of the KBC's own mapping table. Witness how the
> kernel's Tegra KBC driver only contains the plain and fn tables (albeit
> merged into a single array for ease of use), and the input core is what
> interpets e.g. KEY_LEFTSHIFT as a modifier.

Yes, certainly for ctrl I agree that we can simply calculate it.
Although interestingly the other two keyboard drivers in U-Boot use
the same approach as here, so maybe there is a fish hook somewhere (I
have not written this code before).

>
> So specifically what I'd like to see changed in this binding is:
>
> a) We need binding documentation for the Tegra KBC binding, in the same
> style as found in the kernel's Documentation/devicetree/bindings.

OK will do - should I just put that in the cover letter? There is no
such file in U-Boot.

>
> b) The Tegra KBC binding should include only the keycode-plain and
> keycode-fn properties; nothing to do with shift/ctrl/alt/.... (Well,
> also any standardized properties such as compatible, reg, interrupts).

OK. I'm not sure how I specify what happens when you press shift...

>
> c) We need binding documentation for the data that is/could-be common
> across multiple keyboards: i.e. what does each key code value represent;
> which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common
> across (1) all keyboards (2) some standardized meaning for DT that can
> be used by U-Boot, the Linux kernel, etc. Perhaps there is already such
> a standard?

I'm not aware of it.

>
> d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a
> separate binding that can be common to any/all keyboards (probably the
> same document as (b) above). The input to this table should be the raw
> codes from keycode-plain/keycode-fn. The output would be the values sent
> to whatever consumes keyboard input. The naming of these properties
> should make it obvious that it's something not specific to Tegra's KBC,
> and SW oriented. Perhaps something semantically similar to loadkeys' or
> xkbcomp's input, although I haven't looked at those in detail.

While I agree this would be nice, it involves adding a layer of
software into U-Boot which doesn't currently exist (converting key
codes + modifies into ASCII codes).

>
> Linux probably wouldn't use (d), since it already has its own
> SW-oriented methods of setting up such mapping tables. Although perhaps
> in the future the default mappings could be set up from these properties.

Well, and X already has its own, etc.

>
> U-Boot would need (d), since AIUI, there is no handling of such a higher
> layer of mapping tables there right now.
>
> Initially, the code to parse and implement the mappings in (d) could be
> part of the Tegra KBC driver, if there's nowhere else to put it. I
> simply want to ensure that the structure of the mapping table bindings
> doesn't require them to be specific to Tegra KBC, or perpetually
> implemented by the Tegra KBC driver even when/if U-Boot does acquire a
> higher layer that deals with this kind of thing. That's because they're
> SW concepts not part of the HW/HW-binding.

Do you think for the purposes of this series I should modify it to
remove support for the shift key and make the ctrl key use calculated
values? We can then address shift later. That at least puts the
hardware driver up there.

However, even if I do that, the question of what do do about shift
needs to be resolved. At present we have a 128-byte table and a few
lines of code which allows use to support any keyboard type. Whether
we have @ or " above the 2 key, we can support it. If there is a funny
blue hotkey which means 'boot from USB' when you press it and 'boot
from SD' when you press it with shift, we can support it with a
mapping >= ascii 128. It provides maximum flexibility.

The problem is when we have lots of keyboards we may want to do the
translation across all of them to reduce duplicated code in U-Boot.

>From what I can see there will be three keyboard implementations in U-Boot:

- drivers/input/keyboard.c has a translation table
- drivers/input/i8042.c has a translation table also, and supports
German and English
- this driver which has a translation table in the fdt, which seems
like a step forward

We can certainly have a discussion about shift key translation and how
it should work. The same discussion probably applies to un-shifted
keys though, since they may well be printed differently in other
languages. At least with the scheme in this series you can support any
keyboard you like and have complete control over how it behaves.

I think asking for a new input layer in U-Boot. But this does not
exist at present. Don't you think this is taking things a little far?
I am trying to upstream a hardware driver :-)

My reading of the Tegra keyboard driver in the kernel is that it
*could* use the keycode-plain and keycode-fn properties as given in
this series.  They would replace the tegra_kbc_default_keymap[] array.
However, code would need to be added to create that array for use by
the upper layers of linux keyboard input, since presumably it will be
a while before the kernel's drivers/input/keyboard/matrix_keypad.c
supports an fdt-based mapping. It certainly will not use a shift or
ctrl mapping, since these are handled by upper layers of Linux input.

So after all this musing I see two options:

1. Modify this series to remove 'shift' support and make 'ctrl' use a
calculated value (i.e. unlike the other two existing U-Boot drivers).
We lose access to a number of symbols but I could hard-code a mapping
in for the keyboard on Seaboard, say.

2. a. Go with what we have, put a 'u-boot,' prefix on the
keycode-shift property and don't expect the kernel to ever use it.  b.
Start talking on the U-Boot list about the need for a middle input
translation layer and a generic header file which defines key codes in
a standardized way. Then write this layer, get it accepted and
refactor the 3 keyboard drivers to use it.

Thoughts?

Regards,
Simon

>
> --
> nvpublic

Re: [PATCH 4/6] tegra: Add tegra keyboard support

[Stephen Warren]

On 12/08/2011 02:56 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Wed, Dec 7, 2011 at 2:02 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 12/02/2011 07:57 PM, Simon Glass wrote:
>>> Add support for internal matrix keyboard controller for Nvidia Tegra platforms.
>>> This driver uses the fdt decode function to obtain its key codes.
...
>> From a purely HW perspective, the only thing that exists is a single
>> mapping from (row, col) to keycode. I believe that's all the KBC
>> driver's binding should define.
>>
>> I'll amend that slightly: keyboards commonly implement the Fn key
>> internally in HW, since it causes keys to emit completely different raw
>> codes, so I can see this driver wanting both keycode-plain and keycode-fn.
>>
>> However, keycode-shift and keycode-ctrl are purely SW concepts. As such,
>> they shouldn't be part of the KBC's own mapping table. Witness how the
>> kernel's Tegra KBC driver only contains the plain and fn tables (albeit
>> merged into a single array for ease of use), and the input core is what
>> interpets e.g. KEY_LEFTSHIFT as a modifier.
> 
> Yes, certainly for ctrl I agree that we can simply calculate it.
> Although interestingly the other two keyboard drivers in U-Boot use
> the same approach as here, so maybe there is a fish hook somewhere (I
> have not written this code before).

>> So specifically what I'd like to see changed in this binding is:
>>
>> a) We need binding documentation for the Tegra KBC binding, in the same
>> style as found in the kernel's Documentation/devicetree/bindings.
> 
> OK will do - should I just put that in the cover letter? There is no
> such file in U-Boot.

Right now, the canonical location for binding documentation appears to
be the kernel's Documentation/devicetree/bindings/ directory. Perhaps we
should add the documentation there first?

At least, we should post the binding document in the standard kernel
style to the linux-input and devicetree-discuss mailing lists even if it
doesn't get immediately checked in.

I recall from the kernel summit that Grant Likely was thinking of
creating an outside-the-kernel repository for either/both of binding
documentation and .dts/.dtsi. I'm not sure if any progress has been made
there yet.

>> b) The Tegra KBC binding should include only the keycode-plain and
>> keycode-fn properties; nothing to do with shift/ctrl/alt/.... (Well,
>> also any standardized properties such as compatible, reg, interrupts).
> 
> OK. I'm not sure how I specify what happens when you press shift...
> 
>>
>> c) We need binding documentation for the data that is/could-be common
>> across multiple keyboards: i.e. what does each key code value represent;
>> which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common
>> across (1) all keyboards (2) some standardized meaning for DT that can
>> be used by U-Boot, the Linux kernel, etc. Perhaps there is already such
>> a standard?
> 
> I'm not aware of it.

I looked around for any kind of existing keyboard mapping binding, and I
couldn't find anything.

I did find a Samsung matrix KBC binding in the kernel
(Documentation/devicetree/bindings/input/samsung-keypad.txt) which is
conceptually at the same level as having a "plain" table, although no
"fn" table in their case. I prefer the proposed Tegra binding's table
approach rather than having a separate node per key myself.

>> d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a
>> separate binding that can be common to any/all keyboards (probably the
>> same document as (b) above). The input to this table should be the raw
>> codes from keycode-plain/keycode-fn. The output would be the values sent
>> to whatever consumes keyboard input. The naming of these properties
>> should make it obvious that it's something not specific to Tegra's KBC,
>> and SW oriented. Perhaps something semantically similar to loadkeys' or
>> xkbcomp's input, although I haven't looked at those in detail.
> 
> While I agree this would be nice, it involves adding a layer of
> software into U-Boot which doesn't currently exist (converting key
> codes + modifies into ASCII codes).

It doesn't have to be a separate lyaer in U-Boot; the binding just has
to be defined in a way that doesn't tie it to the Tegra KBC driver, so
it can be re-used.

In other words:

Tegra KBC's binding defines:

nvidia,keycode-plain
nvidia,keycode-fn

(these should output "shift" and "ctrl" keycodes for later processing)

Generic key mapping binding defines:

modifier-mapping-shift
modifier-mapping-ctrl

... which take as input the values generated by keycode-plain/keycode-fn
and output the rewritten keycodes.

(the difference here is that the input to those tables is the output
from the nvidia,keycode-foo tables, rather than the raw HW keycodes)

As far as implementation goes, all of that can be handled inside the
Tegra KBC driver for now, so no new layer is required.

I think we should run this plan, and both binding definitions past the
linux-input mailing list; people there will be far more aware of any
previous work in this area, whether this plan makes sense, etc.

...
> I think asking for a new input layer in U-Boot. But this does not
> exist at present. Don't you think this is taking things a little far?
> I am trying to upstream a hardware driver :-)

Yes, I'd like a clear documentation/naming separation between the
HW-specific plain/fn keycode array and the higher-level shift/ctrl
mappings. I certainly am not requesting that you create a separate input
layer in U-Boot to handle those mappings; it can all be done in the
Tegra KBC driver for now.

> My reading of the Tegra keyboard driver in the kernel is that it
> *could* use the keycode-plain and keycode-fn properties as given in
> this series.  They would replace the tegra_kbc_default_keymap[] array.

Yes, that seems quite reasonable.

> However, code would need to be added to create that array for use by
> the upper layers of linux keyboard input, since presumably it will be
> a while before the kernel's drivers/input/keyboard/matrix_keypad.c
> supports an fdt-based mapping. It certainly will not use a shift or
> ctrl mapping, since these are handled by upper layers of Linux input.
> 
> So after all this musing I see two options:
> 
> 1. Modify this series to remove 'shift' support and make 'ctrl' use a
> calculated value (i.e. unlike the other two existing U-Boot drivers).
> We lose access to a number of symbols but I could hard-code a mapping
> in for the keyboard on Seaboard, say.
> 
> 2. a. Go with what we have, put a 'u-boot,' prefix on the
> keycode-shift property and don't expect the kernel to ever use it.  b.
> Start talking on the U-Boot list about the need for a middle input
> translation layer and a generic header file which defines key codes in
> a standardized way. Then write this layer, get it accepted and
> refactor the 3 keyboard drivers to use it.

I'd like to ask for comments in linux-input in case they have any other
ideas, e.g. whether a set of separately documented modifier mapping
properties makes sense. If not, I suppose the following set of
properties would suffice:

nvidia,keycode-plain
nvidia,keycode-fn
u-boot,keycode-shift
u-boot,keycode-ctrl

(although the last two seem to want both nvidia, and u-boot, prefixes)

-- 
nvpublic

Re: [PATCH 4/6] tegra: Add tegra keyboard support

[Simon Glass]

Hi Stephen,

On Mon, Dec 12, 2011 at 10:00 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/08/2011 02:56 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Wed, Dec 7, 2011 at 2:02 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/02/2011 07:57 PM, Simon Glass wrote:
>>>> Add support for internal matrix keyboard controller for Nvidia Tegra platforms.
>>>> This driver uses the fdt decode function to obtain its key codes.
> ...
>>> From a purely HW perspective, the only thing that exists is a single
>>> mapping from (row, col) to keycode. I believe that's all the KBC
>>> driver's binding should define.
>>>
>>> I'll amend that slightly: keyboards commonly implement the Fn key
>>> internally in HW, since it causes keys to emit completely different raw
>>> codes, so I can see this driver wanting both keycode-plain and keycode-fn.
>>>
>>> However, keycode-shift and keycode-ctrl are purely SW concepts. As such,
>>> they shouldn't be part of the KBC's own mapping table. Witness how the
>>> kernel's Tegra KBC driver only contains the plain and fn tables (albeit
>>> merged into a single array for ease of use), and the input core is what
>>> interpets e.g. KEY_LEFTSHIFT as a modifier.
>>
>> Yes, certainly for ctrl I agree that we can simply calculate it.
>> Although interestingly the other two keyboard drivers in U-Boot use
>> the same approach as here, so maybe there is a fish hook somewhere (I
>> have not written this code before).
>
>>> So specifically what I'd like to see changed in this binding is:
>>>
>>> a) We need binding documentation for the Tegra KBC binding, in the same
>>> style as found in the kernel's Documentation/devicetree/bindings.
>>
>> OK will do - should I just put that in the cover letter? There is no
>> such file in U-Boot.
>
> Right now, the canonical location for binding documentation appears to
> be the kernel's Documentation/devicetree/bindings/ directory. Perhaps we
> should add the documentation there first?
>
> At least, we should post the binding document in the standard kernel
> style to the linux-input and devicetree-discuss mailing lists even if it
> doesn't get immediately checked in.
>
> I recall from the kernel summit that Grant Likely was thinking of
> creating an outside-the-kernel repository for either/both of binding
> documentation and .dts/.dtsi. I'm not sure if any progress has been made
> there yet.

Not enough that it is done, but I believe it is happening.

>
>>> b) The Tegra KBC binding should include only the keycode-plain and
>>> keycode-fn properties; nothing to do with shift/ctrl/alt/.... (Well,
>>> also any standardized properties such as compatible, reg, interrupts).
>>
>> OK. I'm not sure how I specify what happens when you press shift...
>>
>>>
>>> c) We need binding documentation for the data that is/could-be common
>>> across multiple keyboards: i.e. what does each key code value represent;
>>> which is KEY_A, KEY_LEFTSHIFT, etc.? These values should be common
>>> across (1) all keyboards (2) some standardized meaning for DT that can
>>> be used by U-Boot, the Linux kernel, etc. Perhaps there is already such
>>> a standard?
>>
>> I'm not aware of it.
>
> I looked around for any kind of existing keyboard mapping binding, and I
> couldn't find anything.
>
> I did find a Samsung matrix KBC binding in the kernel
> (Documentation/devicetree/bindings/input/samsung-keypad.txt) which is
> conceptually at the same level as having a "plain" table, although no
> "fn" table in their case. I prefer the proposed Tegra binding's table
> approach rather than having a separate node per key myself.
>
>>> d) Shift/Ctrl/Alt/... modifier mapping tables should be specified by a
>>> separate binding that can be common to any/all keyboards (probably the
>>> same document as (b) above). The input to this table should be the raw
>>> codes from keycode-plain/keycode-fn. The output would be the values sent
>>> to whatever consumes keyboard input. The naming of these properties
>>> should make it obvious that it's something not specific to Tegra's KBC,
>>> and SW oriented. Perhaps something semantically similar to loadkeys' or
>>> xkbcomp's input, although I haven't looked at those in detail.
>>
>> While I agree this would be nice, it involves adding a layer of
>> software into U-Boot which doesn't currently exist (converting key
>> codes + modifies into ASCII codes).
>
> It doesn't have to be a separate lyaer in U-Boot; the binding just has
> to be defined in a way that doesn't tie it to the Tegra KBC driver, so
> it can be re-used.
>
> In other words:
>
> Tegra KBC's binding defines:
>
> nvidia,keycode-plain
> nvidia,keycode-fn
>
> (these should output "shift" and "ctrl" keycodes for later processing)
>
> Generic key mapping binding defines:
>
> modifier-mapping-shift
> modifier-mapping-ctrl
>
> ... which take as input the values generated by keycode-plain/keycode-fn
> and output the rewritten keycodes.
>
> (the difference here is that the input to those tables is the output
> from the nvidia,keycode-foo tables, rather than the raw HW keycodes)

I'm not sure this passes the 'simple' test. But OK. If we limit it to
characters <128 then we can avoid doubling the size of each mapping.

>
> As far as implementation goes, all of that can be handled inside the
> Tegra KBC driver for now, so no new layer is required.
>
> I think we should run this plan, and both binding definitions past the
> linux-input mailing list; people there will be far more aware of any
> previous work in this area, whether this plan makes sense, etc.
>
> ...
>> I think asking for a new input layer in U-Boot. But this does not
>> exist at present. Don't you think this is taking things a little far?
>> I am trying to upstream a hardware driver :-)
>
> Yes, I'd like a clear documentation/naming separation between the
> HW-specific plain/fn keycode array and the higher-level shift/ctrl
> mappings. I certainly am not requesting that you create a separate input
> layer in U-Boot to handle those mappings; it can all be done in the
> Tegra KBC driver for now.

I would prefer to just leave it as it is and deal with the input layer
when there is demand for it. But I'm OK with your plan.

>
>> My reading of the Tegra keyboard driver in the kernel is that it
>> *could* use the keycode-plain and keycode-fn properties as given in
>> this series.  They would replace the tegra_kbc_default_keymap[] array.
>
> Yes, that seems quite reasonable.
>
>> However, code would need to be added to create that array for use by
>> the upper layers of linux keyboard input, since presumably it will be
>> a while before the kernel's drivers/input/keyboard/matrix_keypad.c
>> supports an fdt-based mapping. It certainly will not use a shift or
>> ctrl mapping, since these are handled by upper layers of Linux input.
>>
>> So after all this musing I see two options:
>>
>> 1. Modify this series to remove 'shift' support and make 'ctrl' use a
>> calculated value (i.e. unlike the other two existing U-Boot drivers).
>> We lose access to a number of symbols but I could hard-code a mapping
>> in for the keyboard on Seaboard, say.
>>
>> 2. a. Go with what we have, put a 'u-boot,' prefix on the
>> keycode-shift property and don't expect the kernel to ever use it.  b.
>> Start talking on the U-Boot list about the need for a middle input
>> translation layer and a generic header file which defines key codes in
>> a standardized way. Then write this layer, get it accepted and
>> refactor the 3 keyboard drivers to use it.
>
> I'd like to ask for comments in linux-input in case they have any other
> ideas, e.g. whether a set of separately documented modifier mapping
> properties makes sense. If not, I suppose the following set of
> properties would suffice:
>
> nvidia,keycode-plain
> nvidia,keycode-fn
> u-boot,keycode-shift
> u-boot,keycode-ctrl
>
> (although the last two seem to want both nvidia, and u-boot, prefixes)

OK. Since this seems to be a kernel issue and Nvidia-specific can I
ask if you can please send this email? I will wait for confirmation
that this is OK before going further.

Regards,
Simon

>
> --
> nvpublic

Re: [PATCH 4/6] tegra: Add tegra keyboard support

[Stephen Warren]

On 12/12/2011 11:10 AM, Simon Glass wrote:
> On Mon, Dec 12, 2011 at 10:00 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 12/08/2011 02:56 PM, Simon Glass wrote:
...
>> I'd like to ask for comments in linux-input in case they have any other
>> ideas, e.g. whether a set of separately documented modifier mapping
>> properties makes sense. If not, I suppose the following set of
>> properties would suffice:
>>
>> nvidia,keycode-plain
>> nvidia,keycode-fn
>> u-boot,keycode-shift
>> u-boot,keycode-ctrl
>>
>> (although the last two seem to want both nvidia, and u-boot, prefixes)
> 
> OK. Since this seems to be a kernel issue and Nvidia-specific can I
> ask if you can please send this email? I will wait for confirmation
> that this is OK before going further.

OK, I'll send the email.

I would like to point out though that this is neither a kernel issue nor
NVIDIA-specific: It's a device tree issue dealing with how to best
represent certain HW features, and it will apply equally to any HW that
has a keyboard with modifier keys; essentially all of them.

-- 
nvpublic

Re: [PATCH 4/6] tegra: Add tegra keyboard support

[Simon Glass]

Hi Stephen,

On Mon, Dec 12, 2011 at 10:31 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/12/2011 11:10 AM, Simon Glass wrote:
>> On Mon, Dec 12, 2011 at 10:00 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/08/2011 02:56 PM, Simon Glass wrote:
> ...
>>> I'd like to ask for comments in linux-input in case they have any other
>>> ideas, e.g. whether a set of separately documented modifier mapping
>>> properties makes sense. If not, I suppose the following set of
>>> properties would suffice:
>>>
>>> nvidia,keycode-plain
>>> nvidia,keycode-fn
>>> u-boot,keycode-shift
>>> u-boot,keycode-ctrl
>>>
>>> (although the last two seem to want both nvidia, and u-boot, prefixes)
>>
>> OK. Since this seems to be a kernel issue and Nvidia-specific can I
>> ask if you can please send this email? I will wait for confirmation
>> that this is OK before going further.
>
> OK, I'll send the email.

Thanks.

>
> I would like to point out though that this is neither a kernel issue nor
> NVIDIA-specific: It's a device tree issue dealing with how to best
> represent certain HW features, and it will apply equally to any HW that
> has a keyboard with modifier keys; essentially all of them.

Sorry I don't quite understand what you mean by that, given your
comments that we should ask on linux-input. While it could apply to
the two existing keyboard drivers, they each do their own thing at
present.

Regards,
Simon

>
> --
> nvpublic