Re: [PATCH 2/7] Input: cros_ec_keyb - add function key support

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Sat, Feb 21, 2026 at 04:37:10PM -0800, Dmitry Torokhov wrote:
> From: Fabio Baltieri <fabiobaltieri@chromium.org>
> 
> 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>
> Link: https://patch.msgid.link/20260211173421.1206478-3-fabiobaltieri@chromium.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

The patch looks good to me, just a few minor nits below.  I don't insist on
these being fixed for this patch to be merged,

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

> +static void cros_ec_keyb_process_key_fn_map(struct cros_ec_keyb *ckdev,
> +					    int row, int col, bool state)
> +{
> +	struct input_dev *idev = ckdev->idev;
> +	const unsigned short *keycodes = idev->keycode;
> +	unsigned int pos, fn_pos;
> +	unsigned int code, fn_code;

Nit: does declaring `code` and `fn_code` as unsigned short make more sense?
Or they can be declared in the same line.

> +
> +	pos = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> +	code = keycodes[pos];
> +
> +	if (code == KEY_FN) {
> +		ckdev->fn_active = state;
> +		if (state) {
> +			ckdev->fn_combo_active = false;
> +		} else if (!ckdev->fn_combo_active) {
> +			/*
> +			 * Send both Fn press and release events if nothing
> +			 * else has been pressed together with Fn.
> +			 */
> +			cros_ec_emit_fn_key(idev, pos);
> +		}
> +		return;
> +	}
> +
> +	fn_pos = MATRIX_SCAN_CODE(row + ckdev->rows, col, ckdev->row_shift);
> +	fn_code = keycodes[fn_pos];
> +
> +	if (state) {
> +		if (ckdev->fn_active) {
> +			ckdev->fn_combo_active = true;
> +			if (!fn_code)
> +				return; /* Discard if no Fn mapping exists */
> +
> +			code = fn_code;
> +			pos = fn_pos;

Nit: assigning `pos` before `code` makes it neater.

> +		}
> +	} else {
> +		/*
> +		 * If the Fn-remapped code is currently pressed, release it.
> +		 * Otherwise, release the standard code (if it was pressed).
> +		 */
> +		if (fn_code && test_bit(fn_code, idev->key)) {
> +			code = fn_code;
> +			pos = fn_pos;

Nit: same here.