| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aYYTgxi0mcPVgpTr@google.com>
Date: Fri, 6 Feb 2026 08:25:14 -0800
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Fabio Baltieri <fabiobaltieri@...omium.org>
Cc: Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>,
Simon Glass <sjg@...omium.org>, linux-input@...r.kernel.org, chrome-platform@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/1] Input: cros_ec_keyb - add function key support
Hi Fabio,
On Mon, Jan 12, 2026 at 09:33:09AM +0000, Fabio Baltieri wrote:
> Add support for handling an Fn button and sending separate keycodes for
> a subset of keys in the matrix defined in the upper half of the keymap.
>
> Signed-off-by: Fabio Baltieri <fabiobaltieri@...omium.org>
> Reviewed-by: Simon Glass <sjg@...omium.org>
> ---
> drivers/input/keyboard/cros_ec_keyb.c | 174 +++++++++++++++++++++++---
> 1 file changed, 158 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 1c6b0461dc35..93540f0c5a33 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -29,6 +29,11 @@
>
> #include <linux/unaligned.h>
>
> +/* Maximum size of the normal key matrix, this is limited by the host command
> + * key_matrix field defined in ec_response_get_next_data_v3
> + */
The comment format for multi-line comments is:
/*
* Line 1
* Line 2
*/
> +#define CROS_EC_KEYBOARD_COLS_MAX 18
> +
> /**
> * struct cros_ec_keyb - Structure representing EC keyboard device
> *
> @@ -44,6 +49,11 @@
> * @bs_idev: The input device for non-matrix buttons and switches (or NULL).
> * @notifier: interrupt event notifier for transport devices
> * @vdata: vivaldi function row data
> + * @has_fn_map: whether the driver uses an fn function-map layer
I do not believe this flag is needed. Always do FN processing. If there
is no FN in the keymap it should work just fine.
> + * @normal_key_status: active normal keys map
> + * @fn_key_status: active function keys map
I do not think you need to track state yourself. A key will be reported
either from FN part of map or from normal one, so when you get a release
for (row, col) you can check if FN-mapped key is active (using
text_bit(<fn-combo-code>, idev->key)) and if it is not active then send
release event for the normal key.
> + * @fn_key_pressed: tracks the function key status
> + * @fn_key_triggered: tracks where any function key fired
Maybe it should be called fn_event_pending? You set it together with
fn_key_pressed and clear if you get any other key press?
> */
> struct cros_ec_keyb {
> unsigned int rows;
> @@ -61,6 +71,12 @@ struct cros_ec_keyb {
> struct notifier_block notifier;
>
> struct vivaldi_data vdata;
> +
> + bool has_fn_map;
> + u8 normal_key_status[CROS_EC_KEYBOARD_COLS_MAX];
> + u8 fn_key_status[CROS_EC_KEYBOARD_COLS_MAX];
> + bool fn_key_pressed;
> + bool fn_key_triggered;
> };
>
> /**
> @@ -166,16 +182,104 @@ static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
> return false;
> }
>
> +/*
> + * Process a function key state change, send an event report if appropriate.
> + */
> +static void cros_ec_keyb_process_fn_key(struct cros_ec_keyb *ckdev,
> + int row, int col, bool state)
> +{
> + struct input_dev *idev = ckdev->idev;
> + int pos = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> +
> + ckdev->fn_key_pressed = state;
> +
> + if (state) {
> + ckdev->fn_key_triggered = false;
> + } else if (!ckdev->fn_key_triggered) {
> + /*
> + * Send the original code if nothing else has been pressed
> + * together with Fn.
> + */
> + input_event(idev, EV_MSC, MSC_SCAN, pos);
> + input_report_key(idev, KEY_FN, true);
> + input_sync(idev);
> +
> + input_event(idev, EV_MSC, MSC_SCAN, pos);
> + input_report_key(idev, KEY_FN, false);
Why do we want this? If you want FN to behave like hardware switch you
probably do not want to send KEY_FN at all?
> + }
> +}
> +
> +/*
> + * Return the Fn code for a normal key row, col combination, optionally set a
> + * position code too.
> + */
> +static unsigned int cros_ec_keyb_fn_code(struct cros_ec_keyb *ckdev,
> + int row, int col, int *pos)
> +{
> + struct input_dev *idev = ckdev->idev;
> + const unsigned short *keycodes = idev->keycode;
> + int fn_pos = MATRIX_SCAN_CODE(row + ckdev->rows, col, ckdev->row_shift);
> +
> + if (pos)
> + *pos = fn_pos;
> +
> + return keycodes[fn_pos];
> +}
> +
> +/*
> + * Process the new state for a single key.
> + */
> +static void cros_ec_keyb_process_one(struct cros_ec_keyb *ckdev,
> + int row, int col, bool state)
> +{
> + struct input_dev *idev = ckdev->idev;
> + const unsigned short *keycodes = idev->keycode;
> + int pos = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> + unsigned int code = keycodes[pos];
> +
> + dev_dbg(ckdev->dev, "changed: [r%d c%d]: byte %02x\n", row, col, state);
> +
> + if (ckdev->has_fn_map) {
> + if (code == KEY_FN)
> + return cros_ec_keyb_process_fn_key(ckdev, row, col, state);
> +
> + if (!state) {
> + if (ckdev->fn_key_status[col] & BIT(row)) {
> + code = cros_ec_keyb_fn_code(ckdev, row, col, &pos);
> +
> + ckdev->fn_key_status[col] &= ~BIT(row);
> + } else if (ckdev->normal_key_status[col] & BIT(row)) {
> + ckdev->normal_key_status[col] &= ~BIT(row);
> + } else {
> + /* Discard, key press code was not sent */
> + return;
> + }
> + } else if (ckdev->fn_key_pressed) {
> + code = cros_ec_keyb_fn_code(ckdev, row, col, &pos);
> +
> + ckdev->fn_key_triggered = true;
> +
> + if (!code)
> + return;
> +
> + ckdev->fn_key_status[col] |= BIT(row);
> + } else {
> + ckdev->normal_key_status[col] |= BIT(row);
> + }
> + }
> +
> + input_event(idev, EV_MSC, MSC_SCAN, pos);
> + input_report_key(idev, code, state);
> +}
>
> /*
> * Compares the new keyboard state to the old one and produces key
> - * press/release events accordingly. The keyboard state is 13 bytes (one byte
> - * per column)
> + * press/release events accordingly. The keyboard state is one byte
> + * per column.
> */
> static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> uint8_t *kb_state, int len)
> {
> - struct input_dev *idev = ckdev->idev;
> int col, row;
> int new_state;
> int old_state;
> @@ -192,20 +296,13 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
>
> for (col = 0; col < ckdev->cols; col++) {
> for (row = 0; row < ckdev->rows; row++) {
> - int pos = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> - const unsigned short *keycodes = idev->keycode;
> -
> new_state = kb_state[col] & (1 << row);
> old_state = ckdev->old_kb_state[col] & (1 << row);
> - if (new_state != old_state) {
> - dev_dbg(ckdev->dev,
> - "changed: [r%d c%d]: byte %02x\n",
> - row, col, new_state);
> -
> - input_event(idev, EV_MSC, MSC_SCAN, pos);
> - input_report_key(idev, keycodes[pos],
> - new_state);
> - }
> +
> + if (new_state == old_state)
> + continue;
> +
> + cros_ec_keyb_process_one(ckdev, row, col, new_state);
> }
> ckdev->old_kb_state[col] = kb_state[col];
> }
> @@ -582,6 +679,43 @@ static void cros_ec_keyb_parse_vivaldi_physmap(struct cros_ec_keyb *ckdev)
> ckdev->vdata.num_function_row_keys = n_physmap;
> }
>
> +/* Returns true if there is a KEY_FN code defined in the normal keymap */
> +static bool cros_ec_keyb_has_fn_key(struct cros_ec_keyb *ckdev)
> +{
> + struct input_dev *idev = ckdev->idev;
> + const unsigned short *keycodes = idev->keycode;
> +
> + for (int row = 0; row < ckdev->rows; row++) {
> + for (int col = 0; col < ckdev->cols; col++) {
> + int pos = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> +
> + if (keycodes[pos] == KEY_FN)
> + return true;
> + }
> + }
> +
> + return false;
> +}
Not needed.
> +
> +/*
> + * Returns true if there is a KEY_FN defined and at least one key in the fn
> + * layer keymap
> + */
> +static bool cros_ec_keyb_has_fn_map(struct cros_ec_keyb *ckdev)
> +{
> + if (!cros_ec_keyb_has_fn_key(ckdev))
> + return false;
> +
> + for (int row = 0; row < ckdev->rows; row++) {
> + for (int col = 0; col < ckdev->cols; col++) {
> + if (cros_ec_keyb_fn_code(ckdev, row, col, NULL) != 0)
> + return true;
> + }
> + }
> +
> + return false;
> +}
I do not think this is needed either.
Thanks.
--
Dmitry
Powered by blists - more mailing lists