Hi Stephen, On Mon, Dec 12, 2011 at 10:00 AM, Stephen Warren <swar...@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 <swar...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot