Re: [PATCH] hw/arm/musicpal: Convert to qemu_add_kbd_event_handler()

[Peter Maydell]

Ping^2 for code review?

thanks
-- PMM

On Fri, 15 Dec 2023 at 14:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Ping for code review, since we're approaching the end of the
> 8.2 freeze ?
>
> thanks
> -- PMM
>
> On Fri, 3 Nov 2023 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > Convert the musicpal key input device to use
> > qemu_add_kbd_event_handler().  This lets us simplify it because we no
> > longer need to track whether we're in the middle of a PS/2 multibyte
> > key sequence.
> >
> > In the conversion we move the keyboard handler registration from init
> > to realize, because devices shouldn't disturb the state of the
> > simulation by doing things like registering input handlers until
> > they're realized, so that device objects can be introspected
> > safely.
> >
> > The behaviour where key-repeat is permitted for the arrow-keys only
> > is intentional (added in commit 7c6ce4baedfcd0c), so we retain it,
> > and add a comment to that effect.
> >
> > This is a migration compatibility break for musicpal.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/musicpal.c | 131 +++++++++++++++++++++-------------------------
> >  1 file changed, 61 insertions(+), 70 deletions(-)
> >
> > diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> > index 9703bfb97fb..e8c1250ab04 100644
> > --- a/hw/arm/musicpal.c
> > +++ b/hw/arm/musicpal.c
> > @@ -1043,20 +1043,6 @@ static const TypeInfo musicpal_gpio_info = {
> >  };
> >
> >  /* Keyboard codes & masks */
> > -#define KEY_RELEASED            0x80
> > -#define KEY_CODE                0x7f
> > -
> > -#define KEYCODE_TAB             0x0f
> > -#define KEYCODE_ENTER           0x1c
> > -#define KEYCODE_F               0x21
> > -#define KEYCODE_M               0x32
> > -
> > -#define KEYCODE_EXTENDED        0xe0
> > -#define KEYCODE_UP              0x48
> > -#define KEYCODE_DOWN            0x50
> > -#define KEYCODE_LEFT            0x4b
> > -#define KEYCODE_RIGHT           0x4d
> > -
> >  #define MP_KEY_WHEEL_VOL       (1 << 0)
> >  #define MP_KEY_WHEEL_VOL_INV   (1 << 1)
> >  #define MP_KEY_WHEEL_NAV       (1 << 2)
> > @@ -1074,67 +1060,66 @@ struct musicpal_key_state {
> >      SysBusDevice parent_obj;
> >      /*< public >*/
> >
> > -    uint32_t kbd_extended;
> >      uint32_t pressed_keys;
> >      qemu_irq out[8];
> >  };
> >
> > -static void musicpal_key_event(void *opaque, int keycode)
> > +static void musicpal_key_event(DeviceState *dev, QemuConsole *src,
> > +                               InputEvent *evt)
> >  {
> > -    musicpal_key_state *s = opaque;
> > +    musicpal_key_state *s = MUSICPAL_KEY(dev);
> > +    InputKeyEvent *key = evt->u.key.data;
> > +    int qcode = qemu_input_key_value_to_qcode(key->key);
> >      uint32_t event = 0;
> >      int i;
> >
> > -    if (keycode == KEYCODE_EXTENDED) {
> > -        s->kbd_extended = 1;
> > -        return;
> > +    switch (qcode) {
> > +    case Q_KEY_CODE_UP:
> > +        event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> > +        break;
> > +
> > +    case Q_KEY_CODE_DOWN:
> > +        event = MP_KEY_WHEEL_NAV;
> > +        break;
> > +
> > +    case Q_KEY_CODE_LEFT:
> > +        event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> > +        break;
> > +
> > +    case Q_KEY_CODE_RIGHT:
> > +        event = MP_KEY_WHEEL_VOL;
> > +        break;
> > +
> > +    case Q_KEY_CODE_F:
> > +        event = MP_KEY_BTN_FAVORITS;
> > +        break;
> > +
> > +    case Q_KEY_CODE_TAB:
> > +        event = MP_KEY_BTN_VOLUME;
> > +        break;
> > +
> > +    case Q_KEY_CODE_RET:
> > +        event = MP_KEY_BTN_NAVIGATION;
> > +        break;
> > +
> > +    case Q_KEY_CODE_M:
> > +        event = MP_KEY_BTN_MENU;
> > +        break;
> >      }
> >
> > -    if (s->kbd_extended) {
> > -        switch (keycode & KEY_CODE) {
> > -        case KEYCODE_UP:
> > -            event = MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_NAV_INV;
> > -            break;
> > -
> > -        case KEYCODE_DOWN:
> > -            event = MP_KEY_WHEEL_NAV;
> > -            break;
> > -
> > -        case KEYCODE_LEFT:
> > -            event = MP_KEY_WHEEL_VOL | MP_KEY_WHEEL_VOL_INV;
> > -            break;
> > -
> > -        case KEYCODE_RIGHT:
> > -            event = MP_KEY_WHEEL_VOL;
> > -            break;
> > -        }
> > -    } else {
> > -        switch (keycode & KEY_CODE) {
> > -        case KEYCODE_F:
> > -            event = MP_KEY_BTN_FAVORITS;
> > -            break;
> > -
> > -        case KEYCODE_TAB:
> > -            event = MP_KEY_BTN_VOLUME;
> > -            break;
> > -
> > -        case KEYCODE_ENTER:
> > -            event = MP_KEY_BTN_NAVIGATION;
> > -            break;
> > -
> > -        case KEYCODE_M:
> > -            event = MP_KEY_BTN_MENU;
> > -            break;
> > -        }
> > -        /* Do not repeat already pressed buttons */
> > -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> > +    /*
> > +     * We allow repeated wheel-events when the arrow keys are held down,
> > +     * but do not repeat already-pressed buttons for the other key inputs.
> > +     */
> > +    if (!(event & (MP_KEY_WHEEL_NAV | MP_KEY_WHEEL_VOL))) {
> > +        if (key->down && (s->pressed_keys & event)) {
> >              event = 0;
> >          }
> >      }
> >
> >      if (event) {
> >          /* Raise GPIO pin first if repeating a key */
> > -        if (!(keycode & KEY_RELEASED) && (s->pressed_keys & event)) {
> > +        if (key->down && (s->pressed_keys & event)) {
> >              for (i = 0; i <= 7; i++) {
> >                  if (event & (1 << i)) {
> >                      qemu_set_irq(s->out[i], 1);
> > @@ -1143,17 +1128,15 @@ static void musicpal_key_event(void *opaque, int 
> > keycode)
> >          }
> >          for (i = 0; i <= 7; i++) {
> >              if (event & (1 << i)) {
> > -                qemu_set_irq(s->out[i], !!(keycode & KEY_RELEASED));
> > +                qemu_set_irq(s->out[i], !key->down);
> >              }
> >          }
> > -        if (keycode & KEY_RELEASED) {
> > -            s->pressed_keys &= ~event;
> > -        } else {
> > +        if (key->down) {
> >              s->pressed_keys |= event;
> > +        } else {
> > +            s->pressed_keys &= ~event;
> >          }
> >      }
> > -
> > -    s->kbd_extended = 0;
> >  }
> >
> >  static void musicpal_key_init(Object *obj)
> > @@ -1162,20 +1145,27 @@ static void musicpal_key_init(Object *obj)
> >      DeviceState *dev = DEVICE(sbd);
> >      musicpal_key_state *s = MUSICPAL_KEY(dev);
> >
> > -    s->kbd_extended = 0;
> >      s->pressed_keys = 0;
> >
> >      qdev_init_gpio_out(dev, s->out, ARRAY_SIZE(s->out));
> > +}
> >
> > -    qemu_add_kbd_event_handler(musicpal_key_event, s);
> > +static const QemuInputHandler musicpal_key_handler = {
> > +    .name = "musicpal_key",
> > +    .mask = INPUT_EVENT_MASK_KEY,
> > +    .event = musicpal_key_event,
> > +};
> > +
> > +static void musicpal_key_realize(DeviceState *dev, Error **errp)
> > +{
> > +    qemu_input_handler_register(dev, &musicpal_key_handler);
> >  }
> >
> >  static const VMStateDescription musicpal_key_vmsd = {
> >      .name = "musicpal_key",
> > -    .version_id = 1,
> > -    .minimum_version_id = 1,
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT32(kbd_extended, musicpal_key_state),
> >          VMSTATE_UINT32(pressed_keys, musicpal_key_state),
> >          VMSTATE_END_OF_LIST()
> >      }
> > @@ -1186,6 +1176,7 @@ static void musicpal_key_class_init(ObjectClass 
> > *klass, void *data)
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >      dc->vmsd = &musicpal_key_vmsd;
> > +    dc->realize = musicpal_key_realize;
> >  }
> >
> >  static const TypeInfo musicpal_key_info = {
> > --
> > 2.34.1
> >