Re: [PATCH v1 2/3] Input: cros_ec_keyb: add function key support

[Fabio Baltieri <fabiobaltieri@chromium.org>]

Hey Simon,

On Thu, Dec 11, 2025 at 06:29:01AM -0700, Simon Glass wrote:
> > @@ -44,6 +52,13 @@
> >   * @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
> > + * @fn_key: coordinate of the function key
> > + * @fn_keymap: array of coordinate and codes for the function keys
> > + * @fn_keymap_len: number of entries in the fn_keymap array
> > + * @fn_key_status: active function keys bitmap
> > + * @normal_key_status: active normal keys bitmap
> > + * @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 +76,14 @@ struct cros_ec_keyb {
> >         struct notifier_block notifier;
> >
> >         struct vivaldi_data vdata;
> > +
> > +       uint32_t fn_key;
> 
> Normally we use u32/u8 these days

Okay, I did notice the file was a bit of a mix, I'll change them in v2.

> 
> > +       uint32_t *fn_keymap;
> > +       int fn_keymap_len;
> > +       uint32_t fn_key_status;
> > +       uint8_t normal_key_status[CROS_EC_KEYBOARD_COLS_MAX];
> > +       bool fn_key_pressed;
> > +       bool fn_key_triggered;
> >  };
> >
> >  /**
> > @@ -166,16 +189,108 @@ static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
> >         return false;
> >  }
> >
> > +static bool cros_ec_key_is(int row, int col, uint32_t key)
> > +{
> > +       if (row == KEY_ROW(key) && col == KEY_COL(key))
> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> > +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->fn_keymap) {
> > +               if (cros_ec_key_is(row, col, ckdev->fn_key)) {
> > +                       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, code, true);
> > +                               input_sync(ckdev->idev);
> 
> What is this function? I might be missing a patch?

input_sync? it sends an EV_SYN, been there from the start, though I
noticed I miss one two lines below, was relying on the rest of the
function to send it but I changed the logic at some point and broke that
path, will fix that.

> 
> > +
> > +                               input_event(idev, EV_MSC, MSC_SCAN, pos);
> > +                               input_report_key(idev, code, false);
> > +                       }
> > +
> > +                       return;
> > +               }
> > +
> > +               if (!state) {
> > +                       /* Key release, may need to release the Fn code */
> > +                       for (int i = 0; i < ckdev->fn_keymap_len; i++) {
> > +                               if (!cros_ec_key_is(row, col,
> > +                                                   ckdev->fn_keymap[i]))
> > +                                       continue;
> > +
> > +                               if ((ckdev->fn_key_status & BIT(i)) == 0)
> > +                                       continue;
> > +
> > +                               code = KEY_VAL(ckdev->fn_keymap[i]);
> > +                               ckdev->fn_key_status &= ~BIT(i);
> > +
> > +                               input_event(idev, EV_MSC, MSC_SCAN, pos);
> > +                               input_report_key(idev, code, state);
> > +
> > +                               return;
> > +                       }
> > +
> > +                       if ((ckdev->normal_key_status[col] & BIT(row)) == 0)
> > +                               /* Discard, key press code was not sent */
> > +                               return;
> > +               } else if (ckdev->fn_key_pressed) {
> > +                       /* Key press while holding Fn */
> > +                       ckdev->fn_key_triggered = true;
> > +
> > +                       for (int i = 0; i < ckdev->fn_keymap_len; i++) {
> > +                               if (!cros_ec_key_is(row, col,
> > +                                                   ckdev->fn_keymap[i]))
> > +                                       continue;
> > +
> > +                               code = KEY_VAL(ckdev->fn_keymap[i]);
> > +                               ckdev->fn_key_status |= BIT(i);
> > +
> > +                               input_event(idev, EV_MSC, MSC_SCAN, pos);
> > +                               input_report_key(idev, code, state);
> > +
> > +                               return;
> > +                       }
> > +
> > +                       /* Do not emit a code if the key is not mapped */
> > +                       return;
> > +               }
> > +       }
> 
> I think this function could do with splitting a bit

Yeah, I don't love it either but there's a lot of logic intertwined in
there, tried to split it myself and ended up breaking stuff, the logic
for the fn key itsel though can go that's a good 16 lines, I'll start
with that, send a v2 and then go from there.

> Can the sandbox driver support this too?

Not sure what you are referring to, can you give me a pointer?

Hey thanks for the review, good to hear from you. :-)

Cheers,
Fabio

-- 
Fabio Baltieri