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

[Dmitry Torokhov]

On Sun, Feb 22, 2026 at 06:46:53PM +0800, Tzung-Bi Shih wrote:
> On Sat, Feb 21, 2026 at 04:37:10PM -0800, Dmitry Torokhov wrote:
> > From: Fabio Baltieri <fabiobaltieri@xxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxx>
> > Reviewed-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > Link: https://patch.msgid.link/20260211173421.1206478-3-fabiobaltieri@xxxxxxxxxxxx
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> 
> 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@xxxxxxxxxx>

Thanks Tzung-Bi.

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

I do not think making temporary variables unsigned short changes
anything, so I will leave it as is.

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

Yep, I'll update before applying.

Thanks.

-- 
Dmitry