[PATCH v3 0/2] Input: cros_ec_keyb: add function key support

[PATCH v3 0/2] Input: cros_ec_keyb: add function key support

[Fabio Baltieri]

Hi, v3 of the cros-ec-keyb fn key support, just few tweaks from the
reviews.

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 (2):
  dt-bindings: google,cros-ec-keyb: add has-fn-map prop
  Input: cros_ec_keyb - add function key support

 .../bindings/input/google,cros-ec-keyb.yaml   |   8 ++
 drivers/input/keyboard/cros_ec_keyb.c         | 136 +++++++++++++++---
 2 files changed, 128 insertions(+), 16 deletions(-)

-- 
2.52.0.351.gbe84eed79e-goog

[PATCH v3 1/2] dt-bindings: google,cros-ec-keyb: add has-fn-map prop

[Fabio Baltieri]

Add binding documentation for the has-fn-map property.

Signed-off-by: Fabio Baltieri <fabiobaltieri@chromium.org>
---
 .../devicetree/bindings/input/google,cros-ec-keyb.yaml    | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
index fefaaf46a240..fa24b1cbc788 100644
--- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
+++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
@@ -44,6 +44,14 @@ properties:
       where the lower 16 bits are reserved. This property is specified only
       when the keyboard has a custom design for the top row keys.
 
+  google,has-fn-map:
+    description: |
+      The keymap has function key layer. This allows defining an extra set of
+      codes that are sent if a key is pressed while the KEY_FN is held pressed
+      as well. The function codes have to be defined in the linux,keymap
+      property with an offset of keypad,num-rows from the normal ones.
+    type: boolean
+
 dependencies:
   function-row-physmap: [ 'linux,keymap' ]
   google,needs-ghost-filter: [ 'linux,keymap' ]
-- 
2.52.0.351.gbe84eed79e-goog

[PATCH v3 2/2] 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 | 136 +++++++++++++++++++++++---
 1 file changed, 120 insertions(+), 16 deletions(-)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 1c6b0461dc35..bc76f61583c3 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 use 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,100 @@ 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.
+ */
+static unsigned int cros_ec_keyb_fn_code(struct cros_ec_keyb *ckdev,
+					 int row, int col)
+{
+	struct input_dev *idev = ckdev->idev;
+	const unsigned short *keycodes = idev->keycode;
+	int pos = MATRIX_SCAN_CODE(row + ckdev->rows, col, ckdev->row_shift);
+
+	return keycodes[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);
+
+				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);
+
+			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 +292,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];
 	}
@@ -597,12 +690,19 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	struct device *dev = ckdev->dev;
 	struct input_dev *idev;
 	const char *phys;
+	unsigned int rows_keymap;
 	int err;
 
 	err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols);
 	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 +735,11 @@ 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,
+	ckdev->has_fn_map = device_property_read_bool(dev, "google,has-fn-map");
+
+	rows_keymap = ckdev->has_fn_map ? ckdev->rows * 2 : ckdev->rows;
+
+	err = matrix_keypad_build_keymap(NULL, NULL, rows_keymap, ckdev->cols,
 					 NULL, idev);
 	if (err) {
 		dev_err(dev, "cannot build key matrix\n");
-- 
2.52.0.351.gbe84eed79e-goog

Re: [PATCH v3 1/2] dt-bindings: google,cros-ec-keyb: add has-fn-map prop

[Krzysztof Kozlowski]

On Wed, Dec 31, 2025 at 02:35:37PM +0000, Fabio Baltieri wrote:
> Add binding documentation for the has-fn-map property.
> 
> Signed-off-by: Fabio Baltieri <fabiobaltieri@chromium.org>
> ---
>  .../devicetree/bindings/input/google,cros-ec-keyb.yaml    | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> index fefaaf46a240..fa24b1cbc788 100644
> --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> @@ -44,6 +44,14 @@ properties:
>        where the lower 16 bits are reserved. This property is specified only
>        when the keyboard has a custom design for the top row keys.
>  
> +  google,has-fn-map:
> +    description: |
> +      The keymap has function key layer. This allows defining an extra set of
> +      codes that are sent if a key is pressed while the KEY_FN is held pressed
> +      as well. The function codes have to be defined in the linux,keymap
> +      property with an offset of keypad,num-rows from the normal ones.
> +    type: boolean

You still did not answer to my previous question, why this is not
deducible from the key map (presence of KEY_FN in the map).

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: google,cros-ec-keyb: add has-fn-map prop

[Fabio Baltieri]

On Mon, Jan 05, 2026 at 08:52:56AM +0100, Krzysztof Kozlowski wrote:
> On Wed, Dec 31, 2025 at 02:35:37PM +0000, Fabio Baltieri wrote:
> > Add binding documentation for the has-fn-map property.
> > 
> > Signed-off-by: Fabio Baltieri <fabiobaltieri@chromium.org>
> > ---
> >  .../devicetree/bindings/input/google,cros-ec-keyb.yaml    | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > index fefaaf46a240..fa24b1cbc788 100644
> > --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > @@ -44,6 +44,14 @@ properties:
> >        where the lower 16 bits are reserved. This property is specified only
> >        when the keyboard has a custom design for the top row keys.
> >  
> > +  google,has-fn-map:
> > +    description: |
> > +      The keymap has function key layer. This allows defining an extra set of
> > +      codes that are sent if a key is pressed while the KEY_FN is held pressed
> > +      as well. The function codes have to be defined in the linux,keymap
> > +      property with an offset of keypad,num-rows from the normal ones.
> > +    type: boolean
> 
> You still did not answer to my previous question, why this is not
> deducible from the key map (presence of KEY_FN in the map).

The driver behaves differently with the fn layer is present, has to make
extra space for the extra codes and enable the logic to use it. I can
certainly detect it in runtime, would have to always allocate the extra
space even if not needed and check not only that there is an FN key but
if there's anything in the second half of the map.

I'm not overly enthusiastic about it, it's a bit wasteful on memory
(probably no big deal, half a kb of RAM I guess) and somewhat less
defensive to misconfigurations and in general I don't like the new logic
to be enabled magically, as a side effect. It'd be extra complexity for
the sake of saving one boolean property, but sure if you think that's
the way to go then I guess I can implement it that way.

Re: [PATCH v3 1/2] dt-bindings: google,cros-ec-keyb: add has-fn-map prop

[Krzysztof Kozlowski]

On Mon, Jan 05, 2026 at 10:55:51AM +0000, Fabio Baltieri wrote:
> On Mon, Jan 05, 2026 at 08:52:56AM +0100, Krzysztof Kozlowski wrote:
> > On Wed, Dec 31, 2025 at 02:35:37PM +0000, Fabio Baltieri wrote:
> > > Add binding documentation for the has-fn-map property.
> > > 
> > > Signed-off-by: Fabio Baltieri <fabiobaltieri@chromium.org>
> > > ---
> > >  .../devicetree/bindings/input/google,cros-ec-keyb.yaml    | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > > index fefaaf46a240..fa24b1cbc788 100644
> > > --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > > +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > > @@ -44,6 +44,14 @@ properties:
> > >        where the lower 16 bits are reserved. This property is specified only
> > >        when the keyboard has a custom design for the top row keys.
> > >  
> > > +  google,has-fn-map:
> > > +    description: |
> > > +      The keymap has function key layer. This allows defining an extra set of
> > > +      codes that are sent if a key is pressed while the KEY_FN is held pressed
> > > +      as well. The function codes have to be defined in the linux,keymap
> > > +      property with an offset of keypad,num-rows from the normal ones.
> > > +    type: boolean
> > 
> > You still did not answer to my previous question, why this is not
> > deducible from the key map (presence of KEY_FN in the map).
> 
> The driver behaves differently with the fn layer is present, has to make
> extra space for the extra codes and enable the logic to use it. I can
> certainly detect it in runtime, would have to always allocate the extra
> space even if not needed and check not only that there is an FN key but
> if there's anything in the second half of the map.
> 
> I'm not overly enthusiastic about it, it's a bit wasteful on memory
> (probably no big deal, half a kb of RAM I guess) and somewhat less
> defensive to misconfigurations and in general I don't like the new logic
> to be enabled magically, as a side effect. It'd be extra complexity for
> the sake of saving one boolean property, but sure if you think that's
> the way to go then I guess I can implement it that way.

Driver logic is not an argument here, we don't care about it. You should
answer why presence of google,has-fn-map in DT makes sense when none of
the keymaps has KEY_FN. Why this is a valid and desired configuration?

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: google,cros-ec-keyb: add has-fn-map prop

[Fabio Baltieri]

On Tue, Jan 06, 2026 at 08:27:10AM +0100, Krzysztof Kozlowski wrote:
> On Mon, Jan 05, 2026 at 10:55:51AM +0000, Fabio Baltieri wrote:
> > On Mon, Jan 05, 2026 at 08:52:56AM +0100, Krzysztof Kozlowski wrote:
> > > On Wed, Dec 31, 2025 at 02:35:37PM +0000, Fabio Baltieri wrote:
> > > > Add binding documentation for the has-fn-map property.
> > > > 
> > > > Signed-off-by: Fabio Baltieri <fabiobaltieri@chromium.org>
> > > > ---
> > > >  .../devicetree/bindings/input/google,cros-ec-keyb.yaml    | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > > > index fefaaf46a240..fa24b1cbc788 100644
> > > > --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > > > +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > > > @@ -44,6 +44,14 @@ properties:
> > > >        where the lower 16 bits are reserved. This property is specified only
> > > >        when the keyboard has a custom design for the top row keys.
> > > >  
> > > > +  google,has-fn-map:
> > > > +    description: |
> > > > +      The keymap has function key layer. This allows defining an extra set of
> > > > +      codes that are sent if a key is pressed while the KEY_FN is held pressed
> > > > +      as well. The function codes have to be defined in the linux,keymap
> > > > +      property with an offset of keypad,num-rows from the normal ones.
> > > > +    type: boolean
> > > 
> > > You still did not answer to my previous question, why this is not
> > > deducible from the key map (presence of KEY_FN in the map).
> > 
> > The driver behaves differently with the fn layer is present, has to make
> > extra space for the extra codes and enable the logic to use it. I can
> > certainly detect it in runtime, would have to always allocate the extra
> > space even if not needed and check not only that there is an FN key but
> > if there's anything in the second half of the map.
> > 
> > I'm not overly enthusiastic about it, it's a bit wasteful on memory
> > (probably no big deal, half a kb of RAM I guess) and somewhat less
> > defensive to misconfigurations and in general I don't like the new logic
> > to be enabled magically, as a side effect. It'd be extra complexity for
> > the sake of saving one boolean property, but sure if you think that's
> > the way to go then I guess I can implement it that way.
> 
> Driver logic is not an argument here, we don't care about it. You should
> answer why presence of google,has-fn-map in DT makes sense when none of
> the keymaps has KEY_FN. Why this is a valid and desired configuration?
> 

Think I answered that, it felt like a clarer design to me. Anyway I got
the message will send a v4 with runtime detection, it's certainly a
valid option and works out for me.