[PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Philip Chen]

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

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

 .../devicetree/bindings/input/google,cros-ec-keyb.yaml     | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
index 8e50c14a9d778..f105eae1cf445 100644
--- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
+++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
@@ -31,6 +31,13 @@ properties:
       if the EC does not have its own logic or hardware for this.
     type: boolean
 
+  google,custom-keyb-top-row:
+    $ref: '/schemas/types.yaml#/definitions/uint16-array'
+    description: |
+      An ordered u16 array containing the action keycode values of the function
+      keys, from left to right. Specified only when the board has a custom
+      design for the top row keys on the keyboard.
+
 required:
   - compatible
 
-- 
2.26.2

[PATCH 2/3] Input: cros_ec_keyb - Support custom top-row keys

[Philip Chen]

The function keys in a keyboard's top row are usually intended for
certain actions such as "Browser back" and "Fullscreen".

As of now, when a top-row key is pressed, cros_ec_keyb sends function
key code (e.g. KEY_F1) instead of action key code (e.g. KEY_BACK) to
applications. Because `linux,keymap` defined in cros-ec-keyboard.dtsi
maps the scanlines of the top-row keys to the function key code.

Therefore, an application can only convert each function key to
different action based on a fixed mapping.

This patch aims to support a more flexible keyboard top-row design. If
a board specifies a custom layout for the top row keys in dt binding,
cros_ec_keyb will explicitly sends action key code to applications when
any top-row key is pressed, so the applications no longer have to make
assumptions.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

 drivers/input/keyboard/cros_ec_keyb.c | 71 +++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index b379ed7628781..c997ec5c5d469 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -27,6 +27,34 @@
 
 #include <asm/unaligned.h>
 
+#define MAX_NUM_TOP_ROW_KEYS   15
+
+/*
+ * Row/column (in the scan matrix) of the function keys (T1-T15)
+ * defined in Chrome OS keyboard spec
+ */
+static const struct key_pos {
+	u8 row;
+	u8 col;
+} top_row_key_pos[] = {
+	{.row = 0, .col = 2},	/* T1 */
+	{.row = 3, .col = 2},	/* T2 */
+	{.row = 2, .col = 2},	/* T3 */
+	{.row = 1, .col = 2},	/* T4 */
+	{.row = 3, .col = 4},	/* T5 */
+	{.row = 2, .col = 4},	/* T6 */
+	{.row = 1, .col = 4},	/* T7 */
+	{.row = 2, .col = 9},	/* T8 */
+	{.row = 1, .col = 9},	/* T9 */
+	{.row = 0, .col = 4},	/* T10 */
+	{.row = 0, .col = 1},	/* T11 */
+	{.row = 1, .col = 5},	/* T12 */
+	{.row = 3, .col = 5},	/* T13 */
+	{.row = 0, .col = 9},	/* T14 */
+	{.row = 0, .col = 11},	/* T15 */
+};
+BUILD_ASSERT(ARRAY_SIZE(top_row_key_pos) == MAX_NUM_TOP_ROW_KEYS);
+
 /**
  * struct cros_ec_keyb - Structure representing EC keyboard device
  *
@@ -42,6 +70,7 @@
  * @idev: The input device for the matrix keys.
  * @bs_idev: The input device for non-matrix buttons and switches (or NULL).
  * @notifier: interrupt event notifier for transport devices
+ * @num_function_row_keys: The number of top row keys in a custom keyboard
  */
 struct cros_ec_keyb {
 	unsigned int rows;
@@ -58,6 +87,8 @@ struct cros_ec_keyb {
 	struct input_dev *idev;
 	struct input_dev *bs_idev;
 	struct notifier_block notifier;
+
+	uint8_t num_function_row_keys;
 };
 
 /**
@@ -511,6 +542,44 @@ static int cros_ec_keyb_register_bs(struct cros_ec_keyb *ckdev)
 	return 0;
 }
 
+/**
+ * cros_ec_keyb_update_custom_keymap
+ *
+ * Update the keymap if the board has custom top row keys.
+ *
+ * @ckdev: The keyboard device
+ */
+
+static void cros_ec_keyb_update_custom_keymap(struct cros_ec_keyb *ckdev)
+{
+	u8 i;
+	u16 code;
+	u16 top_row_key_code[MAX_NUM_TOP_ROW_KEYS] = {0};
+	struct input_dev *idev = ckdev->idev;
+	unsigned short *keymap = idev->keycode;
+
+	if (of_property_read_variable_u16_array(ckdev->dev->of_node,
+						"google,custom-keyb-top-row",
+						top_row_key_code,
+						0,
+						MAX_NUM_TOP_ROW_KEYS) > 0) {
+		for (i = 0; i < MAX_NUM_TOP_ROW_KEYS; i++) {
+			if (!top_row_key_code[i])
+				break;
+			code = MATRIX_SCAN_CODE(top_row_key_pos[i].row,
+						top_row_key_pos[i].col,
+						ckdev->row_shift);
+			/*
+			 * Add the action key code for a top row key
+			 * into the keymap.
+			 */
+			keymap[code] = top_row_key_code[i];
+			__set_bit(keymap[code], idev->keybit);
+		}
+		ckdev->num_function_row_keys = i;
+	}
+}
+
 /**
  * cros_ec_keyb_register_bs - Register matrix keys
  *
@@ -576,6 +645,8 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	input_set_capability(idev, EV_MSC, MSC_SCAN);
 	input_set_drvdata(idev, ckdev);
 	ckdev->idev = idev;
+
+	cros_ec_keyb_update_custom_keymap(ckdev);
 	cros_ec_keyb_compute_valid_keys(ckdev);
 
 	err = input_register_device(ckdev->idev);
-- 
2.26.2

[PATCH 3/3] Input: cros-ec-keyb - Expose function row physical map to userspace

[Philip Chen]

The top-row keys in a keyboard usually have dual functionalities.
E.g. A function key "F1" is also an action key "Browser back".

Therefore, when an application receives an action key code from
a top-row key press, the application needs to know how to correlate
the action key code with the function key code and do the conversion
whenever necessary.

Since the userpace already knows the key scanlines (row/column)
associated with a received key code. Essentially, the userspace only
needs a mapping between the key row/column and the matching physical
location in the top row.

This patch enhances the cros-ec-keyb driver to create such a mapping and
expose it to userspace in the form of a function-row-physmap attribute.
The attribute would be a space separated ordered list of row/column codes,
for the keys in the function row, in left-to-right order.

The attribute will only be present when cros-ec-keyb sends action key
codes for the top-row keys, otherwise the attribute shall not be
visible.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

 drivers/input/keyboard/cros_ec_keyb.c | 59 +++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index c997ec5c5d469..4f2f98bb14663 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -658,6 +658,56 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	return 0;
 }
 
+static ssize_t function_row_physmap_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	ssize_t size = 0;
+	u8 i;
+	u16 code;
+	struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
+
+	if (!ckdev->num_function_row_keys)
+		return 0;
+
+	for (i = 0; i < ckdev->num_function_row_keys; i++) {
+		code = MATRIX_SCAN_CODE(top_row_key_pos[i].row,
+					top_row_key_pos[i].col,
+					ckdev->row_shift);
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%02X ", code);
+	}
+	size += scnprintf(buf + size, PAGE_SIZE - size, "\n");
+
+	return size;
+}
+
+static DEVICE_ATTR_RO(function_row_physmap);
+
+static struct attribute *cros_ec_keyb_attrs[] = {
+	&dev_attr_function_row_physmap.attr,
+	NULL,
+};
+
+static umode_t cros_ec_keyb_attr_is_visible(struct kobject *kobj,
+					    struct attribute *attr,
+					    int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
+
+	if (attr == &dev_attr_function_row_physmap.attr &&
+	    !ckdev->num_function_row_keys)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group cros_ec_keyb_attr_group = {
+	.is_visible = cros_ec_keyb_attr_is_visible,
+	.attrs = cros_ec_keyb_attrs,
+};
+
+
 static int cros_ec_keyb_probe(struct platform_device *pdev)
 {
 	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
@@ -688,6 +738,12 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	err = sysfs_create_group(&dev->kobj, &cros_ec_keyb_attr_group);
+	if (err) {
+		dev_err(dev, "failed to create attributes. err=%d\n", err);
+		return err;
+	}
+
 	ckdev->notifier.notifier_call = cros_ec_keyb_work;
 	err = blocking_notifier_chain_register(&ckdev->ec->event_notifier,
 					       &ckdev->notifier);
@@ -703,6 +759,9 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
 static int cros_ec_keyb_remove(struct platform_device *pdev)
 {
 	struct cros_ec_keyb *ckdev = dev_get_drvdata(&pdev->dev);
+	struct device *dev = &pdev->dev;
+
+	sysfs_remove_group(&dev->kobj, &cros_ec_keyb_attr_group);
 
 	blocking_notifier_chain_unregister(&ckdev->ec->event_notifier,
 					   &ckdev->notifier);
-- 
2.26.2

Re: [PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Dmitry Torokhov]

Hi Philip,

On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:
> 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?

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),
		...
	>;

Note that the last item in the triple is purely cosmetic in this case,
you can change it to 0. It is row and column that are important.

Then the mapping will work properly even if we change keymap, for
example from userspace.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Philip Chen]

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
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Philip,
>
> On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:
> > 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.

> 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.
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.
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.

Thanks.
>
>
> Note that the last item in the triple is purely cosmetic in this case,
> you can change it to 0. It is row and column that are important.
>
> Then the mapping will work properly even if we change keymap, for
> example from userspace.
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Dmitry Torokhov]

On Sat, Jan 02, 2021 at 11:39:34AM -0800, Philip Chen wrote:
> 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
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Philip,
> >
> > On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:
> > > 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.

> 
> > 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.

> 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.
> 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

Re: [PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Philip Chen]

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
<dmitry.torokhov@gmail.com> wrote:
>
> On Sat, Jan 02, 2021 at 11:39:34AM -0800, Philip Chen wrote:
> > 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
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Philip,
> > >
> > > On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:
> > > > 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.
>
> >
> > > 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.
>
> > 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.
> > 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

Re: [PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Philip Chen]

Hi Dmitry,

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

On Sat, Jan 2, 2021 at 8:53 PM Philip Chen <philipchen@chromium.org> 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
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Sat, Jan 02, 2021 at 11:39:34AM -0800, Philip Chen wrote:
> > > 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
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > Hi Philip,
> > > >
> > > > On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:
> > > > > 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.
> >
> > >
> > > > 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.
> >
> > > 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.

> > > 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

Re: [PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Dmitry Torokhov]

Hi Philip,

On Sat, Jan 02, 2021 at 10:11:21PM -0800, Philip Chen wrote:
> Hi Dmitry,
> 
> I have one more question below.
> Could you take a look?
> 
> On Sat, Jan 2, 2021 at 8:53 PM Philip Chen <philipchen@chromium.org> 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
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Sat, Jan 02, 2021 at 11:39:34AM -0800, Philip Chen wrote:
> > > > 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
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > Hi Philip,
> > > > >
> > > > > On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:
> > > > > > 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.
> > >
> > > >
> > > > > 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.
> > >
> > > > 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.

This would mean that we need to hard-code the knowledge of the scan
matrix in the driver and will not allow us to "skip" any keys in the top
row. Given that we did not hard-code the keymap I do not see why we
would want to do it differently with the top row. function-row-physmap
provides greatest flexibility and I do not see any downsides.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] dt-bindings: input: cros-ec-keyb: Add a new property

[Philip Chen]

Hi Dmitry,

On Sun, Jan 3, 2021 at 2:48 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Philip,
>
> On Sat, Jan 02, 2021 at 10:11:21PM -0800, Philip Chen wrote:
> > Hi Dmitry,
> >
> > I have one more question below.
> > Could you take a look?
> >
> > On Sat, Jan 2, 2021 at 8:53 PM Philip Chen <philipchen@chromium.org> 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
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Sat, Jan 02, 2021 at 11:39:34AM -0800, Philip Chen wrote:
> > > > > 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
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > Hi Philip,
> > > > > >
> > > > > > On Mon, Dec 21, 2020 at 05:47:57PM -0800, Philip Chen wrote:
> > > > > > > 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.
> > > >
> > > > >
> > > > > > 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.
> > > >
> > > > > 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.
>
> This would mean that we need to hard-code the knowledge of the scan
> matrix in the driver and will not allow us to "skip" any keys in the top
> row. Given that we did not hard-code the keymap I do not see why we
> would want to do it differently with the top row. function-row-physmap
> provides greatest flexibility and I do not see any downsides.

OK. I updated in v2.
PTAL.
Thanks.

>
> Thanks.
>
> --
> Dmitry