Message-ID: <aYoBTEgrirtcW96F@google.com>
Date: Mon, 9 Feb 2026 15:46:20 +0000
From: Fabio Baltieri <fabiobaltieri@...omium.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
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

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

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.

Powered by blists - more mailing lists