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

On Tue, Jan 12, 2021 at 10:49 PM Stephen Boyd [off-list ref] wrote:

Quoting Philip Chen (2021-01-12 15:55:28)

quoted

On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd [off-list ref] wrote:

quoted

Quoting Philip Chen (2021-01-07 15:42:09)

quoted

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 a left-to-right
order.

The attribute will only be present when the device has a custom design
for the top-row keys.

Is it documented in Documentation/ABI/?

Not yet.
Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`?

Somewhere in testing is fine. I'm not sure if it is a generic proprty
for all keyboards though? What's the path in sysfs?

I wouldn't say it's generic.
It is available in the keyboard device node only when the board has a
custom top-row keyboard design.
The path in sysfs is something like:
/sys/class/input/input0/device/function_row_physmap, where input0 is
cros_ec.

quoted

quoted

quoted

 /**
  * struct cros_ec_keyb - Structure representing EC keyboard device
  *

@@ -42,6 +44,9 @@
  * @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
+ * @function_row_physmap: An array of the encoded rows/columns for the top
+ *                        row function keys, in an order from left to right
+ * @num_function_row_keys: The number of top row keys in a custom keyboard
  */
 struct cros_ec_keyb {
        unsigned int rows;

@@ -58,6 +63,9 @@ struct cros_ec_keyb {
        struct input_dev *idev;
        struct input_dev *bs_idev;
        struct notifier_block notifier;
+
+       u16 function_row_physmap[MAX_NUM_TOP_ROW_KEYS];
+       u8 num_function_row_keys;

Why not size_t?

I usually try to use the minimal required bytes for variables, even
for local ones.
In this case, we only need one byte for num_function_row_keys.
Are there any reasons why size_t is better?

I suppose to indicate that it's an array size. It's not a super strong
argument but the usage of u8 looks like we're trying to save space in a
single structure instance (or maybe a couple if there are a few
keyboards), when for all I know it actually generates worse code because
it has to do some masking operation on the load from memory when it
could just load the value directly into a register.

OK, I'll do size_t.

quoted

quoted

quoted

 };

 /**

@@ -587,6 +613,52 @@ 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;

int i? Why u8? Surely the size of a local variable isn't important.

The same reason as "u8 num_function_row_keys".
Is int better in this case?

Yeah int is better because it's a local variable and nobody cares about
those extra few bytes.

OK, I'll do int.

quoted

quoted

quoted

+       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++)
+               size += scnprintf(buf + size, PAGE_SIZE - size, "%02X ",
+                                 ckdev->function_row_physmap[i]);
+       size += scnprintf(buf + size, PAGE_SIZE - size, "\n");
+
+       return size;

I'd rather see

        ssize_t size = 0;
        int i;
        struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
        u16 *physmap = ckdev->function_row_physmap;

        for (i = 0; i < ckdev->num_function_row_keys; i++)
                size += scnprintf(buf + size, PAGE_SIZE - size,
                                  "%s%02X", size ? " " : "", physmap[i]);

        if (size)
                size += scnprintf(buf + size, PAGE_SIZE - size, "\n");

        return size;

And I wonder if hex_dump_to_buffer() works for this?

It seems to work? I'll give it a try.
If hex_dump_to_buffer() doesn't work, I'll fall back to the
implementation you suggested above.

Ok sounds good.