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@xxxxxxxxxxxx>
> ---
>
> 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?
