Hi Dmitry,

I have one more question below.
Could you take a look?

On Sat, Jan 2, 2021 at 8:53 PM Philip Chen [off-list ref] wrote:

Hi Dmitry,

I see.
I'll update these patch sets shortly based on your suggestion.
Thanks.

On Sat, Jan 2, 2021 at 1:04 PM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Sat, Jan 02, 2021 at 11:39:34AM -0800, Philip Chen wrote:

quoted

Hi Dmitry,

Thanks for reviewing my patch over the holiday season.
Please check my CIL.

On Mon, Dec 28, 2020 at 10:18 PM Dmitry Torokhov
[off-list ref] wrote:

quoted

Hi Philip,

On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:

quoted

This patch adds a new property `google,custom-keyb-top-row` to the
device tree for the custom keyboard top row design.

Why don't we use the property we have for the same purpose in atkbd.c?
I.e. function-row-physmap?

Because this property serves a different purpose than function-row-physmap.
`function-row-physmap` basically links the scancode to the physical
position in the top row.
`google,custom-keyb-top-row` aims at specifying the board-specific
keyboard top row design associated with the action codes.

In x86 path, the board-specific keyboard top row design associated
with the action codes is exposed from coreboot to kernel through
"linux,keymap" acpi table.
When coreboot generates this acpi table, it asks EC to provide this
information, since we add the board-specific top-row-design in EC
codebase.
(E.g. https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/board/jinlon/board.c#396)

In ARM, we don't plan to involve EC in the vivaldi support stack.
So `google,custom-keyb-top-row` DT property is our replacement for the
board-specific top-row-design in x86 EC codebase.

I disagree with this decision. We already have "linux,keymap" property
that is supposed to hold accurate keymap for the device in question,
there should be no need to introduce yet another property to adjust the
keymap to reflect the reality. If a device uses "non classic" ChromeOS
top row it should not be using the default keymap from
arch/arm/boot/dts/cros-ec-keyboard.dtsi but supply its own. You can
consider splitting the keymap into generic lower portion and the top row
and moving them into an .h file so they can be easily reused.

quoted

quoted

Also, instead of specifying keycodes in this array we should use
combination of row and column identifying keys, like this:

        function-row-physmap = <
                MATRIX_KEY(0x00, 0x02, KEY_F1),
                MATRIX_KEY(0x03, 0x02, KEY_F2),
                ...
        >;

This mapping between row/column to function keycode is fixed for all
Chrome OS devices.

*for now* The mapping for the rest of the keyboard has also stayed
static, but we still did not hardcode this information in the driver but
rather used DT property to pass it into the kernel.

quoted

So we don't really need to host this information in DT.
Instead, I plan to hardcode this information in cros_ec_keyb.c.
(Please see the array "top_row_key_pos[]" in my next patch: "[2/3]
Input: cros_ec_keyb - Support custom top-row keys".)

The only thing that could make the function-row-physmap file different
among boards is the number of top row keys.

Given the reason above, can we just add `num-top-row-keys` property
instead of the whole `function-row-physmap`?
I think this is the only thing cros_ec_keyb needs to know to generate
the board-specific function-row-physmap file for the userspace.

quoted

quoted

But this information can be derived from the length of
`google,custom-keyb-top-row`.
So we don't need a separate DT property for it.

I am sorry, but I must insist. We need to have:

- accurate keymap in linux,keymap property
- a separate property describing location of top row keys in terms of
  rows and columns (whether we reuse MATRIX_KEY or define another macro
  I do not really care).

Thanks.

--
Dmitry