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

[Stephen Boyd <swboyd@chromium.org>]

Quoting Philip Chen (2021-01-07 15:42:09)
> 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/?

> 
> Signed-off-by: Philip Chen <philipchen@chromium.org>
> ---
> 
> Changes in v4:
> - replace sysfs_create_group() with devm_device_add_group()
> - remove an unused member in struct cros_ec_keyb
> 
> Changes in v3:
> - parse `function-row-physmap` from DT earlier, when we probe
>   cros_ec_keyb, and then store the extracted info in struct cros_ec_keyb.
> 
> Changes in v2:
> - create function-row-physmap file in sysfs by parsing
>   `function-row-physmap` property from DT
> - assume the device already has a correct keymap to reflect the custom
>   top-row keys (if they exist)
> 
>  drivers/input/keyboard/cros_ec_keyb.c | 78 +++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index b379ed7628781..75d1cb29734ce 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -27,6 +27,8 @@
>  
>  #include <asm/unaligned.h>
>  
> +#define MAX_NUM_TOP_ROW_KEYS   15
> +

Ah, the binding could say max is 15 then.

>  /**
>   * 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?

>  };
>  
>  /**
> @@ -527,6 +535,8 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
>         struct input_dev *idev;
>         const char *phys;
>         int err;
> +       u32 top_row_key_pos[MAX_NUM_TOP_ROW_KEYS] = {0};
> +       u8 i;
>  
>         err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols);
>         if (err)
> @@ -578,6 +588,22 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
>         ckdev->idev = idev;
>         cros_ec_keyb_compute_valid_keys(ckdev);
>  
> +       if (of_property_read_variable_u32_array(dev->of_node,
> +                                               "function-row-physmap",
> +                                               top_row_key_pos,
> +                                               0,
> +                                               MAX_NUM_TOP_ROW_KEYS) > 0) {
> +               for (i = 0; i < MAX_NUM_TOP_ROW_KEYS; i++) {

Can we deindent this once with of_property_for_each_u32()?

> +                       if (!top_row_key_pos[i])
> +                               break;
> +                       ckdev->function_row_physmap[i] = MATRIX_SCAN_CODE(
> +                                               KEY_ROW(top_row_key_pos[i]),
> +                                               KEY_COL(top_row_key_pos[i]),

And then have a local variable for top_row_key_pos[i] so this is
shorter.

> +                                               ckdev->row_shift);
> +               }
> +               ckdev->num_function_row_keys = i;
> +       }
> +
>         err = input_register_device(ckdev->idev);
>         if (err) {
>                 dev_err(dev, "cannot register input device\n");
> @@ -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.

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