[U-Boot] [PATCH 4/6] tegra: Add tegra keyboard support

[Simon Glass]

Hi Stephen,

On Wed, Dec 7, 2011 at 2:02 PM, Stephen Warren <swarren at 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.
>
>> +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.