Re: [PATCH v5 1/1] Input: cros_ec_keyb - add function key support

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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@chromium.org>
> Reviewed-by: Simon Glass <sjg@chromium.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