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

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

[Fabio Baltieri]

Changes from v4:
  - just a comment tweak

Changes from v3:
  - implemented fn layer runtime detection
  - tweaked cros_ec_keyb_fn_code to return back the correct position
    code

Changes from v2:
  - renamed the dt property to use-fn-map, dropped the example
  - added few function comments
  - added a helper for obtaining the fn code
  - reordered, dt patch first

Changes from v1:
  - change struct to short types
  - refactored the fn key handling in its own function
  - changed props to use the google, prefix
  - reworked the properties to use an overlay map rather than a
    dedicated one

Fabio Baltieri (1):
  Input: cros_ec_keyb - add function key support

 drivers/input/keyboard/cros_ec_keyb.c | 174 +++++++++++++++++++++++---
 1 file changed, 158 insertions(+), 16 deletions(-)

-- 
2.52.0.457.g6b5491de43-goog

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

[Fabio Baltieri]

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
+ */
+#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
+ * @normal_key_status: active normal keys map
+ * @fn_key_status: active function keys map
+ * @fn_key_pressed: tracks the function key status
+ * @fn_key_triggered: tracks where any function key fired
  */
 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);
+	}
+}
+
+/*
+ * 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;
+}
+
+/*
+ * 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;
+}
+
 /**
  * cros_ec_keyb_register_matrix - Register matrix keys
  *
@@ -603,6 +737,12 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	if (err)
 		return err;
 
+	if (ckdev->cols > CROS_EC_KEYBOARD_COLS_MAX) {
+		dev_err(dev, "keypad,num-columns too large: %d (max: %d)\n",
+			ckdev->cols, CROS_EC_KEYBOARD_COLS_MAX);
+		return -EINVAL;
+	}
+
 	ckdev->valid_keys = devm_kzalloc(dev, ckdev->cols, GFP_KERNEL);
 	if (!ckdev->valid_keys)
 		return -ENOMEM;
@@ -635,7 +775,7 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	ckdev->ghost_filter = device_property_read_bool(dev,
 					"google,needs-ghost-filter");
 
-	err = matrix_keypad_build_keymap(NULL, NULL, ckdev->rows, ckdev->cols,
+	err = matrix_keypad_build_keymap(NULL, NULL, ckdev->rows * 2, ckdev->cols,
 					 NULL, idev);
 	if (err) {
 		dev_err(dev, "cannot build key matrix\n");
@@ -650,6 +790,8 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	cros_ec_keyb_compute_valid_keys(ckdev);
 	cros_ec_keyb_parse_vivaldi_physmap(ckdev);
 
+	ckdev->has_fn_map = cros_ec_keyb_has_fn_map(ckdev);
+
 	err = input_register_device(ckdev->idev);
 	if (err) {
 		dev_err(dev, "cannot register input device\n");
-- 
2.52.0.457.g6b5491de43-goog

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

[Dmitry Torokhov]

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

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

[Fabio Baltieri]

On Fri, Feb 06, 2026 at 08:25:14AM -0800, Dmitry Torokhov wrote:
> 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
>  */

Sure will fix it.

> 
> > +#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.

The problem is that if there is an Fn key and a keymap, hence we process
the Fn keys in the kernel, then we don't send the Fn events, but we
currently have devices deployed with an Fn key where the key is handled
by the userspace and they expect KEY_FN events to be emitted, so if I
let the "fn keymap" logic kick in it unconditionally it would cause a
regression for existing devices.

> 
> > + * @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.

Cool, yes that's the case, was able to get rid of both these two, thanks
for the pointer.

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

Sounds good I'll rename it.

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

Yeah normally you wouldn't want FN events, the problem though is that if
you don't send any events at all then a user may find a blanked screen,
hit the Fn key expecting it to unblank and instead it doesn't happen and
they assume the device is dead. My laptop does that too (in fact it
sends a KEY_WAKEUP).

Also I guess this behavior ultimately allows the userspace to bind
actions to the both the Fn key and Fn key function (imagine an UI like
"press the key to bind this event", if you emit Fn then it woulc capture
that one, if you never emit anything you just can't use it).

> 
> > +	}
> > +}
> > +
> > +/*
> > + * 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.

See my comment about has_fn_map.

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

[Dmitry Torokhov]

On Mon, Feb 09, 2026 at 03:46:20PM +0000, Fabio Baltieri wrote:
> On Fri, Feb 06, 2026 at 08:25:14AM -0800, Dmitry Torokhov wrote:
> > 
> > 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.
> 
> The problem is that if there is an Fn key and a keymap, hence we process
> the Fn keys in the kernel, then we don't send the Fn events, but we
> currently have devices deployed with an Fn key where the key is handled
> by the userspace and they expect KEY_FN events to be emitted, so if I
> let the "fn keymap" logic kick in it unconditionally it would cause a
> regression for existing devices.

Hmm, I see. Then I think we really need to have it as a device property,
because keymap can be manipulated at runtime, so depending on it to
switch processing seems weird.

It is like autorepeat, either device configuration asks for it, or it
does not...

Thanks.

-- 
Dmitry

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

[Fabio Baltieri]

On Mon, Feb 09, 2026 at 10:20:52AM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 09, 2026 at 03:46:20PM +0000, Fabio Baltieri wrote:
> > On Fri, Feb 06, 2026 at 08:25:14AM -0800, Dmitry Torokhov wrote:
> > > 
> > > 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.
> > 
> > The problem is that if there is an Fn key and a keymap, hence we process
> > the Fn keys in the kernel, then we don't send the Fn events, but we
> > currently have devices deployed with an Fn key where the key is handled
> > by the userspace and they expect KEY_FN events to be emitted, so if I
> > let the "fn keymap" logic kick in it unconditionally it would cause a
> > regression for existing devices.
> 
> Hmm, I see. Then I think we really need to have it as a device property,
> because keymap can be manipulated at runtime, so depending on it to
> switch processing seems weird.
> 
> It is like autorepeat, either device configuration asks for it, or it
> does not...

Ok, the DT folks were fairly explicit about not wanting anything that
even remotely looks like configuration into dt. Right now the behavior
changes based on what's in the keymap, which I think is fine.

I see the keymap can be manipulated in runtime but then I guess I could
just install a custom hook to idev->setkeycode, recompute
cros_ec_keyb_has_fn_map() and then call input_default_getkeycode()?
I'd have to make that function public but then it'd automatically change
the behavior in runtime as keycodes are defined/undefined.

Would that be acceptable?

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

[Dmitry Torokhov]

On Mon, Feb 09, 2026 at 07:33:05PM +0000, Fabio Baltieri wrote:
> On Mon, Feb 09, 2026 at 10:20:52AM -0800, Dmitry Torokhov wrote:
> > On Mon, Feb 09, 2026 at 03:46:20PM +0000, Fabio Baltieri wrote:
> > > On Fri, Feb 06, 2026 at 08:25:14AM -0800, Dmitry Torokhov wrote:
> > > > 
> > > > 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.
> > > 
> > > The problem is that if there is an Fn key and a keymap, hence we process
> > > the Fn keys in the kernel, then we don't send the Fn events, but we
> > > currently have devices deployed with an Fn key where the key is handled
> > > by the userspace and they expect KEY_FN events to be emitted, so if I
> > > let the "fn keymap" logic kick in it unconditionally it would cause a
> > > regression for existing devices.
> > 
> > Hmm, I see. Then I think we really need to have it as a device property,
> > because keymap can be manipulated at runtime, so depending on it to
> > switch processing seems weird.
> > 
> > It is like autorepeat, either device configuration asks for it, or it
> > does not...
> 
> Ok, the DT folks were fairly explicit about not wanting anything that
> even remotely looks like configuration into dt. Right now the behavior
> changes based on what's in the keymap, which I think is fine.
> 
> I see the keymap can be manipulated in runtime but then I guess I could
> just install a custom hook to idev->setkeycode, recompute
> cros_ec_keyb_has_fn_map() and then call input_default_getkeycode()?
> I'd have to make that function public but then it'd automatically change
> the behavior in runtime as keycodes are defined/undefined.
> 
> Would that be acceptable?

OK, let's see how it will look like. Exporting
input_default_setkeycode() should be fine, we just need to stick
lockdep_assert_held() there to make sure it is not called without event
lock being held.

Thanks.

-- 
Dmitry