* [PATCH v4 0/2] next-kbd: convert to use qemu_input_handler_register()
@ 2024-11-06 12:09 Mark Cave-Ayland
2024-11-06 12:09 ` [PATCH v4 1/2] " Mark Cave-Ayland
2024-11-06 12:09 ` [PATCH v4 2/2] ui/input-legacy.c: remove unused legacy qemu_add_kbd_event_handler() function Mark Cave-Ayland
0 siblings, 2 replies; 14+ messages in thread
From: Mark Cave-Ayland @ 2024-11-06 12:09 UTC (permalink / raw)
To: peter.maydell, huth, berrange, qemu-devel
This series converts the next-kbd device to use
qemu_input_handler_register(), and then removes the now-unused legacy
qemu_add_kbd_event_handler() function.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
v4:
- Rebase onto master
- Remove extra zero entries from qcode_to_nextkbd_keycode[] array
- Move position of shift key logic as suggested by Daniel
- Add R-B tags from Thomas, Alex and Phil
v3:
- Rebase onto master
- Use Q_KEY_CODE__MAX for the size of qcode_to_nextkbd_keycode() array
- Fix shift key logic using example provided by Thomas
- Fix spelling of NEXTKBD_NO_KEY
- Add R-B tag from Alex for patch 2
v2:
- Rebase onto master
- Add patch 2 to remove the legacy qemu_add_kbd_event_handler()
function
Mark Cave-Ayland (2):
next-kbd: convert to use qemu_input_handler_register()
ui/input-legacy.c: remove unused legacy qemu_add_kbd_event_handler()
function
hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++---------------
include/ui/console.h | 2 -
ui/input-legacy.c | 37 ----------
3 files changed, 108 insertions(+), 94 deletions(-)
--
2.39.5
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-06 12:09 [PATCH v4 0/2] next-kbd: convert to use qemu_input_handler_register() Mark Cave-Ayland @ 2024-11-06 12:09 ` Mark Cave-Ayland 2024-11-06 12:21 ` Daniel P. Berrangé 2024-11-06 13:00 ` BALATON Zoltan 2024-11-06 12:09 ` [PATCH v4 2/2] ui/input-legacy.c: remove unused legacy qemu_add_kbd_event_handler() function Mark Cave-Ayland 1 sibling, 2 replies; 14+ messages in thread From: Mark Cave-Ayland @ 2024-11-06 12:09 UTC (permalink / raw) To: peter.maydell, huth, berrange, qemu-devel Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler() function to use qemu_input_handler_register(). Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Reviewed-by: Thomas Huth <huth@tuxfamily.org> --- hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- 1 file changed, 108 insertions(+), 55 deletions(-) diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c index bc67810f31..377917c1ec 100644 --- a/hw/m68k/next-kbd.c +++ b/hw/m68k/next-kbd.c @@ -68,7 +68,6 @@ struct NextKBDState { uint16_t shift; }; -static void queue_code(void *opaque, int code); /* lots of magic numbers here */ static uint32_t kbd_read_byte(void *opaque, hwaddr addr) @@ -166,68 +165,75 @@ static const MemoryRegionOps kbd_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -static void nextkbd_event(void *opaque, int ch) -{ - /* - * Will want to set vars for caps/num lock - * if (ch & 0x80) -> key release - * there's also e0 escaped scancodes that might need to be handled - */ - queue_code(opaque, ch); -} - -static const unsigned char next_keycodes[128] = { - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 +#define NEXTKBD_NO_KEY 0xff + +static const int qcode_to_nextkbd_keycode[] = { + /* Make sure future additions are automatically set to NEXTKBD_NO_KEY */ + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, + + [Q_KEY_CODE_ESC] = 0x49, + [Q_KEY_CODE_1] = 0x4a, + [Q_KEY_CODE_2] = 0x4b, + [Q_KEY_CODE_3] = 0x4c, + [Q_KEY_CODE_4] = 0x4d, + [Q_KEY_CODE_5] = 0x50, + [Q_KEY_CODE_6] = 0x4f, + [Q_KEY_CODE_7] = 0x4e, + [Q_KEY_CODE_8] = 0x1e, + [Q_KEY_CODE_9] = 0x1f, + [Q_KEY_CODE_0] = 0x20, + [Q_KEY_CODE_MINUS] = 0x1d, + [Q_KEY_CODE_EQUAL] = 0x1c, + [Q_KEY_CODE_BACKSPACE] = 0x1b, + + [Q_KEY_CODE_Q] = 0x42, + [Q_KEY_CODE_W] = 0x43, + [Q_KEY_CODE_E] = 0x44, + [Q_KEY_CODE_R] = 0x45, + [Q_KEY_CODE_T] = 0x48, + [Q_KEY_CODE_Y] = 0x47, + [Q_KEY_CODE_U] = 0x46, + [Q_KEY_CODE_I] = 0x06, + [Q_KEY_CODE_O] = 0x07, + [Q_KEY_CODE_P] = 0x08, + [Q_KEY_CODE_RET] = 0x2a, + [Q_KEY_CODE_A] = 0x39, + [Q_KEY_CODE_S] = 0x3a, + + [Q_KEY_CODE_D] = 0x3b, + [Q_KEY_CODE_F] = 0x3c, + [Q_KEY_CODE_G] = 0x3d, + [Q_KEY_CODE_H] = 0x40, + [Q_KEY_CODE_J] = 0x3f, + [Q_KEY_CODE_K] = 0x3e, + [Q_KEY_CODE_L] = 0x2d, + [Q_KEY_CODE_SEMICOLON] = 0x2c, + [Q_KEY_CODE_APOSTROPHE] = 0x2b, + [Q_KEY_CODE_GRAVE_ACCENT] = 0x26, + [Q_KEY_CODE_Z] = 0x31, + [Q_KEY_CODE_X] = 0x32, + [Q_KEY_CODE_C] = 0x33, + [Q_KEY_CODE_V] = 0x34, + + [Q_KEY_CODE_B] = 0x35, + [Q_KEY_CODE_N] = 0x37, + [Q_KEY_CODE_M] = 0x36, + [Q_KEY_CODE_COMMA] = 0x2e, + [Q_KEY_CODE_DOT] = 0x2f, + [Q_KEY_CODE_SLASH] = 0x30, + + [Q_KEY_CODE_SPC] = 0x38, }; -static void queue_code(void *opaque, int code) +static void nextkbd_put_keycode(NextKBDState *s, int keycode) { - NextKBDState *s = NEXTKBD(opaque); KBDQueue *q = &s->queue; - int key = code & KD_KEYMASK; - int release = code & 0x80; - static int ext; - - if (code == 0xE0) { - ext = 1; - } - - if (code == 0x2A || code == 0x1D || code == 0x36) { - if (code == 0x2A) { - s->shift = KD_LSHIFT; - } else if (code == 0x36) { - s->shift = KD_RSHIFT; - ext = 0; - } else if (code == 0x1D && !ext) { - s->shift = KD_LCOMM; - } else if (code == 0x1D && ext) { - ext = 0; - s->shift = KD_RCOMM; - } - return; - } else if (code == (0x2A | 0x80) || code == (0x1D | 0x80) || - code == (0x36 | 0x80)) { - s->shift = 0; - return; - } if (q->count >= KBD_QUEUE_SIZE) { return; } - q->data[q->wptr] = next_keycodes[key] | release; - + q->data[q->wptr] = keycode; if (++q->wptr == KBD_QUEUE_SIZE) { q->wptr = 0; } @@ -241,6 +247,53 @@ static void queue_code(void *opaque, int code) /* s->update_irq(s->update_arg, 1); */ } +static void nextkbd_event(DeviceState *dev, QemuConsole *src, InputEvent *evt) +{ + NextKBDState *s = NEXTKBD(dev); + int qcode, keycode; + bool key_down = evt->u.key.data->down; + + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { + return; + } + + /* Shift key currently has no keycode, so handle separately */ + if (qcode == Q_KEY_CODE_SHIFT) { + if (key_down) { + s->shift |= KD_LSHIFT; + } else { + s->shift &= ~KD_LSHIFT; + } + } + + if (qcode == Q_KEY_CODE_SHIFT_R) { + if (key_down) { + s->shift |= KD_RSHIFT; + } else { + s->shift &= ~KD_RSHIFT; + } + } + + keycode = qcode_to_nextkbd_keycode[qcode]; + if (keycode == NEXTKBD_NO_KEY) { + return; + } + + /* If key release event, create keyboard break code */ + if (!key_down) { + keycode |= 0x80; + } + + nextkbd_put_keycode(s, keycode); +} + +static const QemuInputHandler nextkbd_handler = { + .name = "QEMU NeXT Keyboard", + .mask = INPUT_EVENT_MASK_KEY, + .event = nextkbd_event, +}; + static void nextkbd_reset(DeviceState *dev) { NextKBDState *nks = NEXTKBD(dev); @@ -256,7 +309,7 @@ static void nextkbd_realize(DeviceState *dev, Error **errp) memory_region_init_io(&s->mr, OBJECT(dev), &kbd_ops, s, "next.kbd", 0x1000); sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr); - qemu_add_kbd_event_handler(nextkbd_event, s); + qemu_input_handler_register(dev, &nextkbd_handler); } static const VMStateDescription nextkbd_vmstate = { -- 2.39.5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-06 12:09 ` [PATCH v4 1/2] " Mark Cave-Ayland @ 2024-11-06 12:21 ` Daniel P. Berrangé 2024-11-06 13:00 ` BALATON Zoltan 1 sibling, 0 replies; 14+ messages in thread From: Daniel P. Berrangé @ 2024-11-06 12:21 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: peter.maydell, huth, qemu-devel On Wed, Nov 06, 2024 at 12:09:27PM +0000, Mark Cave-Ayland wrote: > Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler() > function to use qemu_input_handler_register(). > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Reviewed-by: Thomas Huth <huth@tuxfamily.org> > --- > hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- > 1 file changed, 108 insertions(+), 55 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-06 12:09 ` [PATCH v4 1/2] " Mark Cave-Ayland 2024-11-06 12:21 ` Daniel P. Berrangé @ 2024-11-06 13:00 ` BALATON Zoltan 2024-11-06 15:50 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 14+ messages in thread From: BALATON Zoltan @ 2024-11-06 13:00 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: peter.maydell, huth, berrange, qemu-devel On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: > Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler() > function to use qemu_input_handler_register(). > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Reviewed-by: Thomas Huth <huth@tuxfamily.org> > --- > hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- > 1 file changed, 108 insertions(+), 55 deletions(-) > > diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c > index bc67810f31..377917c1ec 100644 > --- a/hw/m68k/next-kbd.c > +++ b/hw/m68k/next-kbd.c > @@ -68,7 +68,6 @@ struct NextKBDState { > uint16_t shift; > }; > > -static void queue_code(void *opaque, int code); > > /* lots of magic numbers here */ > static uint32_t kbd_read_byte(void *opaque, hwaddr addr) > @@ -166,68 +165,75 @@ static const MemoryRegionOps kbd_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > -static void nextkbd_event(void *opaque, int ch) > -{ > - /* > - * Will want to set vars for caps/num lock > - * if (ch & 0x80) -> key release > - * there's also e0 escaped scancodes that might need to be handled > - */ > - queue_code(opaque, ch); > -} > - > -static const unsigned char next_keycodes[128] = { > - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, > - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, > - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, > - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, > - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, > - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, > - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, > - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 > +#define NEXTKBD_NO_KEY 0xff Now you don't need this 0xff define any more because you can use 0 as no key value then the [0 ... Q_KEY_CODE__MAX] init below can also be dropped because static variables are 0 init automatically. Regards, BALATON Zoltan > +static const int qcode_to_nextkbd_keycode[] = { > + /* Make sure future additions are automatically set to NEXTKBD_NO_KEY */ > + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, > + > + [Q_KEY_CODE_ESC] = 0x49, > + [Q_KEY_CODE_1] = 0x4a, > + [Q_KEY_CODE_2] = 0x4b, > + [Q_KEY_CODE_3] = 0x4c, > + [Q_KEY_CODE_4] = 0x4d, > + [Q_KEY_CODE_5] = 0x50, > + [Q_KEY_CODE_6] = 0x4f, > + [Q_KEY_CODE_7] = 0x4e, > + [Q_KEY_CODE_8] = 0x1e, > + [Q_KEY_CODE_9] = 0x1f, > + [Q_KEY_CODE_0] = 0x20, > + [Q_KEY_CODE_MINUS] = 0x1d, > + [Q_KEY_CODE_EQUAL] = 0x1c, > + [Q_KEY_CODE_BACKSPACE] = 0x1b, > + > + [Q_KEY_CODE_Q] = 0x42, > + [Q_KEY_CODE_W] = 0x43, > + [Q_KEY_CODE_E] = 0x44, > + [Q_KEY_CODE_R] = 0x45, > + [Q_KEY_CODE_T] = 0x48, > + [Q_KEY_CODE_Y] = 0x47, > + [Q_KEY_CODE_U] = 0x46, > + [Q_KEY_CODE_I] = 0x06, > + [Q_KEY_CODE_O] = 0x07, > + [Q_KEY_CODE_P] = 0x08, > + [Q_KEY_CODE_RET] = 0x2a, > + [Q_KEY_CODE_A] = 0x39, > + [Q_KEY_CODE_S] = 0x3a, > + > + [Q_KEY_CODE_D] = 0x3b, > + [Q_KEY_CODE_F] = 0x3c, > + [Q_KEY_CODE_G] = 0x3d, > + [Q_KEY_CODE_H] = 0x40, > + [Q_KEY_CODE_J] = 0x3f, > + [Q_KEY_CODE_K] = 0x3e, > + [Q_KEY_CODE_L] = 0x2d, > + [Q_KEY_CODE_SEMICOLON] = 0x2c, > + [Q_KEY_CODE_APOSTROPHE] = 0x2b, > + [Q_KEY_CODE_GRAVE_ACCENT] = 0x26, > + [Q_KEY_CODE_Z] = 0x31, > + [Q_KEY_CODE_X] = 0x32, > + [Q_KEY_CODE_C] = 0x33, > + [Q_KEY_CODE_V] = 0x34, > + > + [Q_KEY_CODE_B] = 0x35, > + [Q_KEY_CODE_N] = 0x37, > + [Q_KEY_CODE_M] = 0x36, > + [Q_KEY_CODE_COMMA] = 0x2e, > + [Q_KEY_CODE_DOT] = 0x2f, > + [Q_KEY_CODE_SLASH] = 0x30, > + > + [Q_KEY_CODE_SPC] = 0x38, > }; > > -static void queue_code(void *opaque, int code) > +static void nextkbd_put_keycode(NextKBDState *s, int keycode) > { > - NextKBDState *s = NEXTKBD(opaque); > KBDQueue *q = &s->queue; > - int key = code & KD_KEYMASK; > - int release = code & 0x80; > - static int ext; > - > - if (code == 0xE0) { > - ext = 1; > - } > - > - if (code == 0x2A || code == 0x1D || code == 0x36) { > - if (code == 0x2A) { > - s->shift = KD_LSHIFT; > - } else if (code == 0x36) { > - s->shift = KD_RSHIFT; > - ext = 0; > - } else if (code == 0x1D && !ext) { > - s->shift = KD_LCOMM; > - } else if (code == 0x1D && ext) { > - ext = 0; > - s->shift = KD_RCOMM; > - } > - return; > - } else if (code == (0x2A | 0x80) || code == (0x1D | 0x80) || > - code == (0x36 | 0x80)) { > - s->shift = 0; > - return; > - } > > if (q->count >= KBD_QUEUE_SIZE) { > return; > } > > - q->data[q->wptr] = next_keycodes[key] | release; > - > + q->data[q->wptr] = keycode; > if (++q->wptr == KBD_QUEUE_SIZE) { > q->wptr = 0; > } > @@ -241,6 +247,53 @@ static void queue_code(void *opaque, int code) > /* s->update_irq(s->update_arg, 1); */ > } > > +static void nextkbd_event(DeviceState *dev, QemuConsole *src, InputEvent *evt) > +{ > + NextKBDState *s = NEXTKBD(dev); > + int qcode, keycode; > + bool key_down = evt->u.key.data->down; > + > + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); > + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { > + return; > + } > + > + /* Shift key currently has no keycode, so handle separately */ > + if (qcode == Q_KEY_CODE_SHIFT) { > + if (key_down) { > + s->shift |= KD_LSHIFT; > + } else { > + s->shift &= ~KD_LSHIFT; > + } > + } > + > + if (qcode == Q_KEY_CODE_SHIFT_R) { > + if (key_down) { > + s->shift |= KD_RSHIFT; > + } else { > + s->shift &= ~KD_RSHIFT; > + } > + } > + > + keycode = qcode_to_nextkbd_keycode[qcode]; > + if (keycode == NEXTKBD_NO_KEY) { > + return; > + } > + > + /* If key release event, create keyboard break code */ > + if (!key_down) { > + keycode |= 0x80; > + } > + > + nextkbd_put_keycode(s, keycode); > +} > + > +static const QemuInputHandler nextkbd_handler = { > + .name = "QEMU NeXT Keyboard", > + .mask = INPUT_EVENT_MASK_KEY, > + .event = nextkbd_event, > +}; > + > static void nextkbd_reset(DeviceState *dev) > { > NextKBDState *nks = NEXTKBD(dev); > @@ -256,7 +309,7 @@ static void nextkbd_realize(DeviceState *dev, Error **errp) > memory_region_init_io(&s->mr, OBJECT(dev), &kbd_ops, s, "next.kbd", 0x1000); > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr); > > - qemu_add_kbd_event_handler(nextkbd_event, s); > + qemu_input_handler_register(dev, &nextkbd_handler); > } > > static const VMStateDescription nextkbd_vmstate = { > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-06 13:00 ` BALATON Zoltan @ 2024-11-06 15:50 ` Philippe Mathieu-Daudé 2024-11-06 20:32 ` BALATON Zoltan 0 siblings, 1 reply; 14+ messages in thread From: Philippe Mathieu-Daudé @ 2024-11-06 15:50 UTC (permalink / raw) To: BALATON Zoltan, Mark Cave-Ayland Cc: peter.maydell, huth, berrange, qemu-devel On 6/11/24 13:00, BALATON Zoltan wrote: > On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >> Convert the next-kbd device from the legacy UI >> qemu_add_kbd_event_handler() >> function to use qemu_input_handler_register(). >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >> --- >> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >> 1 file changed, 108 insertions(+), 55 deletions(-) >> -static const unsigned char next_keycodes[128] = { >> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >> +#define NEXTKBD_NO_KEY 0xff > > Now you don't need this 0xff define any more because you can use 0 as no > key value then the [0 ... Q_KEY_CODE__MAX] init below can also be > dropped because static variables are 0 init automatically. Whether 0 or 0xff is best for NO_KEY, I don't know. However, definitions are useful when reviewing ... > > Regards, > BALATON Zoltan > >> +static const int qcode_to_nextkbd_keycode[] = { >> + /* Make sure future additions are automatically set to >> NEXTKBD_NO_KEY */ >> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >> + >> + [Q_KEY_CODE_ESC] = 0x49, >> + [Q_KEY_CODE_1] = 0x4a, >> + [Q_KEY_CODE_2] = 0x4b, >> + [Q_KEY_CODE_3] = 0x4c, >> + [Q_KEY_CODE_4] = 0x4d, [...] >> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, >> InputEvent *evt) >> +{ >> + NextKBDState *s = NEXTKBD(dev); >> + int qcode, keycode; >> + bool key_down = evt->u.key.data->down; >> + >> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >> + return; >> + } >> + >> + /* Shift key currently has no keycode, so handle separately */ >> + if (qcode == Q_KEY_CODE_SHIFT) { >> + if (key_down) { >> + s->shift |= KD_LSHIFT; >> + } else { >> + s->shift &= ~KD_LSHIFT; >> + } >> + } >> + >> + if (qcode == Q_KEY_CODE_SHIFT_R) { >> + if (key_down) { >> + s->shift |= KD_RSHIFT; >> + } else { >> + s->shift &= ~KD_RSHIFT; >> + } >> + } >> + >> + keycode = qcode_to_nextkbd_keycode[qcode]; >> + if (keycode == NEXTKBD_NO_KEY) { ... here ^ >> + return; >> + } >> + >> + /* If key release event, create keyboard break code */ >> + if (!key_down) { >> + keycode |= 0x80; >> + } >> + >> + nextkbd_put_keycode(s, keycode); >> +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-06 15:50 ` Philippe Mathieu-Daudé @ 2024-11-06 20:32 ` BALATON Zoltan 2024-11-08 10:05 ` Thomas Huth 0 siblings, 1 reply; 14+ messages in thread From: BALATON Zoltan @ 2024-11-06 20:32 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Mark Cave-Ayland, peter.maydell, huth, berrange, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3675 bytes --] On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: > On 6/11/24 13:00, BALATON Zoltan wrote: >> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>> Convert the next-kbd device from the legacy UI >>> qemu_add_kbd_event_handler() >>> function to use qemu_input_handler_register(). >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>> --- >>> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >>> 1 file changed, 108 insertions(+), 55 deletions(-) > > >>> -static const unsigned char next_keycodes[128] = { >>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>> +#define NEXTKBD_NO_KEY 0xff >> >> Now you don't need this 0xff define any more because you can use 0 as no >> key value then the [0 ... Q_KEY_CODE__MAX] init below can also be dropped >> because static variables are 0 init automatically. > > Whether 0 or 0xff is best for NO_KEY, I don't know. > However, definitions are useful when reviewing ... > >> >> Regards, >> BALATON Zoltan >> >>> +static const int qcode_to_nextkbd_keycode[] = { >>> + /* Make sure future additions are automatically set to NEXTKBD_NO_KEY >>> */ >>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>> + >>> + [Q_KEY_CODE_ESC] = 0x49, >>> + [Q_KEY_CODE_1] = 0x4a, >>> + [Q_KEY_CODE_2] = 0x4b, >>> + [Q_KEY_CODE_3] = 0x4c, >>> + [Q_KEY_CODE_4] = 0x4d, > [...] > >>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, InputEvent >>> *evt) >>> +{ >>> + NextKBDState *s = NEXTKBD(dev); >>> + int qcode, keycode; >>> + bool key_down = evt->u.key.data->down; >>> + >>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>> + return; >>> + } >>> + >>> + /* Shift key currently has no keycode, so handle separately */ >>> + if (qcode == Q_KEY_CODE_SHIFT) { >>> + if (key_down) { >>> + s->shift |= KD_LSHIFT; >>> + } else { >>> + s->shift &= ~KD_LSHIFT; >>> + } >>> + } >>> + >>> + if (qcode == Q_KEY_CODE_SHIFT_R) { >>> + if (key_down) { >>> + s->shift |= KD_RSHIFT; >>> + } else { >>> + s->shift &= ~KD_RSHIFT; >>> + } >>> + } >>> + >>> + keycode = qcode_to_nextkbd_keycode[qcode]; >>> + if (keycode == NEXTKBD_NO_KEY) { > > ... here ^ I this case !keycode is pretty self explanatory IMO. Regards, BALATON Zoltan >>> + return; >>> + } >>> + >>> + /* If key release event, create keyboard break code */ >>> + if (!key_down) { >>> + keycode |= 0x80; >>> + } >>> + >>> + nextkbd_put_keycode(s, keycode); >>> +} > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-06 20:32 ` BALATON Zoltan @ 2024-11-08 10:05 ` Thomas Huth 2024-11-08 12:26 ` Mark Cave-Ayland 2024-11-08 13:13 ` BALATON Zoltan 0 siblings, 2 replies; 14+ messages in thread From: Thomas Huth @ 2024-11-08 10:05 UTC (permalink / raw) To: BALATON Zoltan, Philippe Mathieu-Daudé Cc: Mark Cave-Ayland, peter.maydell, huth, berrange, qemu-devel On 06/11/2024 21.32, BALATON Zoltan wrote: > On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: >> On 6/11/24 13:00, BALATON Zoltan wrote: >>> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>>> Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler() >>>> function to use qemu_input_handler_register(). >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>>> --- >>>> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >>>> 1 file changed, 108 insertions(+), 55 deletions(-) >> >> >>>> -static const unsigned char next_keycodes[128] = { >>>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>>> +#define NEXTKBD_NO_KEY 0xff >>> >>> Now you don't need this 0xff define any more because you can use 0 as no >>> key value then the [0 ... Q_KEY_CODE__MAX] init below can also be dropped >>> because static variables are 0 init automatically. >> >> Whether 0 or 0xff is best for NO_KEY, I don't know. >> However, definitions are useful when reviewing ... >> >>> >>> Regards, >>> BALATON Zoltan >>> >>>> +static const int qcode_to_nextkbd_keycode[] = { >>>> + /* Make sure future additions are automatically set to >>>> NEXTKBD_NO_KEY */ >>>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>>> + >>>> + [Q_KEY_CODE_ESC] = 0x49, >>>> + [Q_KEY_CODE_1] = 0x4a, >>>> + [Q_KEY_CODE_2] = 0x4b, >>>> + [Q_KEY_CODE_3] = 0x4c, >>>> + [Q_KEY_CODE_4] = 0x4d, >> [...] >> >>>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, >>>> InputEvent *evt) >>>> +{ >>>> + NextKBDState *s = NEXTKBD(dev); >>>> + int qcode, keycode; >>>> + bool key_down = evt->u.key.data->down; >>>> + >>>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>>> + return; >>>> + } >>>> + >>>> + /* Shift key currently has no keycode, so handle separately */ >>>> + if (qcode == Q_KEY_CODE_SHIFT) { >>>> + if (key_down) { >>>> + s->shift |= KD_LSHIFT; >>>> + } else { >>>> + s->shift &= ~KD_LSHIFT; >>>> + } >>>> + } >>>> + >>>> + if (qcode == Q_KEY_CODE_SHIFT_R) { >>>> + if (key_down) { >>>> + s->shift |= KD_RSHIFT; >>>> + } else { >>>> + s->shift &= ~KD_RSHIFT; >>>> + } >>>> + } >>>> + >>>> + keycode = qcode_to_nextkbd_keycode[qcode]; >>>> + if (keycode == NEXTKBD_NO_KEY) { >> >> ... here ^ > > I this case !keycode is pretty self explanatory IMO. Ok, I'll pick up the patch with this change added on top: diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c --- a/hw/m68k/next-kbd.c +++ b/hw/m68k/next-kbd.c @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -#define NEXTKBD_NO_KEY 0xff - static const int qcode_to_nextkbd_keycode[] = { - /* Make sure future additions are automatically set to NEXTKBD_NO_KEY */ - [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, - [Q_KEY_CODE_ESC] = 0x49, [Q_KEY_CODE_1] = 0x4a, [Q_KEY_CODE_2] = 0x4b, @@ -276,7 +271,7 @@ static void nextkbd_event(DeviceState *dev, QemuConsole *src, InputEvent *evt) } keycode = qcode_to_nextkbd_keycode[qcode]; - if (keycode == NEXTKBD_NO_KEY) { + if (!keycode) { return; } Thomas ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-08 10:05 ` Thomas Huth @ 2024-11-08 12:26 ` Mark Cave-Ayland 2024-11-08 13:13 ` BALATON Zoltan 1 sibling, 0 replies; 14+ messages in thread From: Mark Cave-Ayland @ 2024-11-08 12:26 UTC (permalink / raw) To: Thomas Huth, BALATON Zoltan, Philippe Mathieu-Daudé Cc: peter.maydell, huth, berrange, qemu-devel On 08/11/2024 10:05, Thomas Huth wrote: > On 06/11/2024 21.32, BALATON Zoltan wrote: >> On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: >>> On 6/11/24 13:00, BALATON Zoltan wrote: >>>> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>>>> Convert the next-kbd device from the legacy UI qemu_add_kbd_event_handler() >>>>> function to use qemu_input_handler_register(). >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>>>> --- >>>>> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >>>>> 1 file changed, 108 insertions(+), 55 deletions(-) >>> >>> >>>>> -static const unsigned char next_keycodes[128] = { >>>>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>>>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>>>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>>>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>>>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>>>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>>>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>>>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>>>> +#define NEXTKBD_NO_KEY 0xff >>>> >>>> Now you don't need this 0xff define any more because you can use 0 as no key >>>> value then the [0 ... Q_KEY_CODE__MAX] init below can also be dropped because >>>> static variables are 0 init automatically. >>> >>> Whether 0 or 0xff is best for NO_KEY, I don't know. >>> However, definitions are useful when reviewing ... >>> >>>> >>>> Regards, >>>> BALATON Zoltan >>>> >>>>> +static const int qcode_to_nextkbd_keycode[] = { >>>>> + /* Make sure future additions are automatically set to NEXTKBD_NO_KEY */ >>>>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>>>> + >>>>> + [Q_KEY_CODE_ESC] = 0x49, >>>>> + [Q_KEY_CODE_1] = 0x4a, >>>>> + [Q_KEY_CODE_2] = 0x4b, >>>>> + [Q_KEY_CODE_3] = 0x4c, >>>>> + [Q_KEY_CODE_4] = 0x4d, >>> [...] >>> >>>>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, InputEvent *evt) >>>>> +{ >>>>> + NextKBDState *s = NEXTKBD(dev); >>>>> + int qcode, keycode; >>>>> + bool key_down = evt->u.key.data->down; >>>>> + >>>>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>>>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>>>> + return; >>>>> + } >>>>> + >>>>> + /* Shift key currently has no keycode, so handle separately */ >>>>> + if (qcode == Q_KEY_CODE_SHIFT) { >>>>> + if (key_down) { >>>>> + s->shift |= KD_LSHIFT; >>>>> + } else { >>>>> + s->shift &= ~KD_LSHIFT; >>>>> + } >>>>> + } >>>>> + >>>>> + if (qcode == Q_KEY_CODE_SHIFT_R) { >>>>> + if (key_down) { >>>>> + s->shift |= KD_RSHIFT; >>>>> + } else { >>>>> + s->shift &= ~KD_RSHIFT; >>>>> + } >>>>> + } >>>>> + >>>>> + keycode = qcode_to_nextkbd_keycode[qcode]; >>>>> + if (keycode == NEXTKBD_NO_KEY) { >>> >>> ... here ^ >> >> I this case !keycode is pretty self explanatory IMO. > > Ok, I'll pick up the patch with this change added on top: > > diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c > --- a/hw/m68k/next-kbd.c > +++ b/hw/m68k/next-kbd.c > @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > -#define NEXTKBD_NO_KEY 0xff > - > static const int qcode_to_nextkbd_keycode[] = { > - /* Make sure future additions are automatically set to NEXTKBD_NO_KEY */ > - [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, > - > [Q_KEY_CODE_ESC] = 0x49, > [Q_KEY_CODE_1] = 0x4a, > [Q_KEY_CODE_2] = 0x4b, > @@ -276,7 +271,7 @@ static void nextkbd_event(DeviceState *dev, QemuConsole *src, > InputEvent *evt) > } > > keycode = qcode_to_nextkbd_keycode[qcode]; > - if (keycode == NEXTKBD_NO_KEY) { > + if (!keycode) { > return; > } > > Thomas Thank you Thomas, greatly appreciated! ATB, Mark. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-08 10:05 ` Thomas Huth 2024-11-08 12:26 ` Mark Cave-Ayland @ 2024-11-08 13:13 ` BALATON Zoltan 2024-11-08 13:24 ` BALATON Zoltan 2024-11-08 15:45 ` Philippe Mathieu-Daudé 1 sibling, 2 replies; 14+ messages in thread From: BALATON Zoltan @ 2024-11-08 13:13 UTC (permalink / raw) To: Thomas Huth Cc: Philippe Mathieu-Daudé, Mark Cave-Ayland, peter.maydell, huth, berrange, qemu-devel [-- Attachment #1: Type: text/plain, Size: 5107 bytes --] On Fri, 8 Nov 2024, Thomas Huth wrote: > On 06/11/2024 21.32, BALATON Zoltan wrote: >> On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: >>> On 6/11/24 13:00, BALATON Zoltan wrote: >>>> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>>>> Convert the next-kbd device from the legacy UI >>>>> qemu_add_kbd_event_handler() >>>>> function to use qemu_input_handler_register(). >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>>>> --- >>>>> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >>>>> 1 file changed, 108 insertions(+), 55 deletions(-) >>> >>> >>>>> -static const unsigned char next_keycodes[128] = { >>>>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>>>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>>>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>>>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>>>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>>>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>>>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>>>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>>>> +#define NEXTKBD_NO_KEY 0xff >>>> >>>> Now you don't need this 0xff define any more because you can use 0 as no >>>> key value then the [0 ... Q_KEY_CODE__MAX] init below can also be dropped >>>> because static variables are 0 init automatically. >>> >>> Whether 0 or 0xff is best for NO_KEY, I don't know. >>> However, definitions are useful when reviewing ... >>> >>>> >>>> Regards, >>>> BALATON Zoltan >>>> >>>>> +static const int qcode_to_nextkbd_keycode[] = { >>>>> + /* Make sure future additions are automatically set to >>>>> NEXTKBD_NO_KEY */ >>>>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>>>> + >>>>> + [Q_KEY_CODE_ESC] = 0x49, >>>>> + [Q_KEY_CODE_1] = 0x4a, >>>>> + [Q_KEY_CODE_2] = 0x4b, >>>>> + [Q_KEY_CODE_3] = 0x4c, >>>>> + [Q_KEY_CODE_4] = 0x4d, >>> [...] >>> >>>>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, >>>>> InputEvent *evt) >>>>> +{ >>>>> + NextKBDState *s = NEXTKBD(dev); >>>>> + int qcode, keycode; >>>>> + bool key_down = evt->u.key.data->down; >>>>> + >>>>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>>>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>>>> + return; >>>>> + } >>>>> + >>>>> + /* Shift key currently has no keycode, so handle separately */ >>>>> + if (qcode == Q_KEY_CODE_SHIFT) { >>>>> + if (key_down) { >>>>> + s->shift |= KD_LSHIFT; >>>>> + } else { >>>>> + s->shift &= ~KD_LSHIFT; >>>>> + } >>>>> + } >>>>> + >>>>> + if (qcode == Q_KEY_CODE_SHIFT_R) { >>>>> + if (key_down) { >>>>> + s->shift |= KD_RSHIFT; >>>>> + } else { >>>>> + s->shift &= ~KD_RSHIFT; >>>>> + } >>>>> + } >>>>> + >>>>> + keycode = qcode_to_nextkbd_keycode[qcode]; >>>>> + if (keycode == NEXTKBD_NO_KEY) { >>> >>> ... here ^ >> >> I this case !keycode is pretty self explanatory IMO. > > Ok, I'll pick up the patch with this change added on top: > > diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c > --- a/hw/m68k/next-kbd.c > +++ b/hw/m68k/next-kbd.c > @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > -#define NEXTKBD_NO_KEY 0xff > - > static const int qcode_to_nextkbd_keycode[] = { > - /* Make sure future additions are automatically set to NEXTKBD_NO_KEY */ > - [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, Thinking about it more, removing this may make the array smaller so we'd either need some max value define (or get it something like qcode_to_nextkbd_keycode[ARRAY_SIZE(qcode_to_nextkbd_keycode) - 1] or so) and check if qcode is not > than that or declare the array as [Q_KEY_CODE__MAX] to make sure we're not trying to access values after the end. Maybe it's simplest to do qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] as this is not much wasted space, unless this can't overflow for some other reason I don't know about. Regards, BALATON Zoltan > - > [Q_KEY_CODE_ESC] = 0x49, > [Q_KEY_CODE_1] = 0x4a, > [Q_KEY_CODE_2] = 0x4b, > @@ -276,7 +271,7 @@ static void nextkbd_event(DeviceState *dev, QemuConsole > *src, InputEvent *evt) > } > keycode = qcode_to_nextkbd_keycode[qcode]; > - if (keycode == NEXTKBD_NO_KEY) { > + if (!keycode) { > return; > } > Thomas > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-08 13:13 ` BALATON Zoltan @ 2024-11-08 13:24 ` BALATON Zoltan 2024-11-08 13:36 ` Thomas Huth 2024-11-08 15:45 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 14+ messages in thread From: BALATON Zoltan @ 2024-11-08 13:24 UTC (permalink / raw) To: Thomas Huth Cc: Philippe Mathieu-Daudé, Mark Cave-Ayland, peter.maydell, huth, berrange, qemu-devel [-- Attachment #1: Type: text/plain, Size: 5563 bytes --] On Fri, 8 Nov 2024, BALATON Zoltan wrote: > On Fri, 8 Nov 2024, Thomas Huth wrote: >> On 06/11/2024 21.32, BALATON Zoltan wrote: >>> On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: >>>> On 6/11/24 13:00, BALATON Zoltan wrote: >>>>> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>>>>> Convert the next-kbd device from the legacy UI >>>>>> qemu_add_kbd_event_handler() >>>>>> function to use qemu_input_handler_register(). >>>>>> >>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>>>>> --- >>>>>> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >>>>>> 1 file changed, 108 insertions(+), 55 deletions(-) >>>> >>>> >>>>>> -static const unsigned char next_keycodes[128] = { >>>>>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>>>>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>>>>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>>>>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>>>>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>>>>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>>>>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>>>>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>>>>> +#define NEXTKBD_NO_KEY 0xff >>>>> >>>>> Now you don't need this 0xff define any more because you can use 0 as no >>>>> key value then the [0 ... Q_KEY_CODE__MAX] init below can also be >>>>> dropped because static variables are 0 init automatically. >>>> >>>> Whether 0 or 0xff is best for NO_KEY, I don't know. >>>> However, definitions are useful when reviewing ... >>>> >>>>> >>>>> Regards, >>>>> BALATON Zoltan >>>>> >>>>>> +static const int qcode_to_nextkbd_keycode[] = { >>>>>> + /* Make sure future additions are automatically set to >>>>>> NEXTKBD_NO_KEY */ >>>>>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>>>>> + >>>>>> + [Q_KEY_CODE_ESC] = 0x49, >>>>>> + [Q_KEY_CODE_1] = 0x4a, >>>>>> + [Q_KEY_CODE_2] = 0x4b, >>>>>> + [Q_KEY_CODE_3] = 0x4c, >>>>>> + [Q_KEY_CODE_4] = 0x4d, >>>> [...] >>>> >>>>>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, >>>>>> InputEvent *evt) >>>>>> +{ >>>>>> + NextKBDState *s = NEXTKBD(dev); >>>>>> + int qcode, keycode; >>>>>> + bool key_down = evt->u.key.data->down; >>>>>> + >>>>>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>>>>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>>>>> + return; >>>>>> + } Never mind, there's already a check for that here so it should be OK. (Does this still work with SHIFTs not having a value in the array? Maybe they happen to be lower than the array size but maybe it's better to move this check lower?) Regards, BALATON Zoltan >>>>>> + >>>>>> + /* Shift key currently has no keycode, so handle separately */ >>>>>> + if (qcode == Q_KEY_CODE_SHIFT) { >>>>>> + if (key_down) { >>>>>> + s->shift |= KD_LSHIFT; >>>>>> + } else { >>>>>> + s->shift &= ~KD_LSHIFT; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (qcode == Q_KEY_CODE_SHIFT_R) { >>>>>> + if (key_down) { >>>>>> + s->shift |= KD_RSHIFT; >>>>>> + } else { >>>>>> + s->shift &= ~KD_RSHIFT; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + keycode = qcode_to_nextkbd_keycode[qcode]; >>>>>> + if (keycode == NEXTKBD_NO_KEY) { >>>> >>>> ... here ^ >>> >>> I this case !keycode is pretty self explanatory IMO. >> >> Ok, I'll pick up the patch with this change added on top: >> >> diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c >> --- a/hw/m68k/next-kbd.c >> +++ b/hw/m68k/next-kbd.c >> @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = { >> .endianness = DEVICE_NATIVE_ENDIAN, >> }; >> -#define NEXTKBD_NO_KEY 0xff >> - >> static const int qcode_to_nextkbd_keycode[] = { >> - /* Make sure future additions are automatically set to NEXTKBD_NO_KEY >> */ >> - [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, > > Thinking about it more, removing this may make the array smaller so we'd > either need some max value define (or get it something like > qcode_to_nextkbd_keycode[ARRAY_SIZE(qcode_to_nextkbd_keycode) - 1] or so) and > check if qcode is not > than that or declare the array as [Q_KEY_CODE__MAX] > to make sure we're not trying to access values after the end. Maybe it's > simplest to do qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] as this is not much > wasted space, unless this can't overflow for some other reason I don't know > about. > > Regards, > BALATON Zoltan > >> - >> [Q_KEY_CODE_ESC] = 0x49, >> [Q_KEY_CODE_1] = 0x4a, >> [Q_KEY_CODE_2] = 0x4b, >> @@ -276,7 +271,7 @@ static void nextkbd_event(DeviceState *dev, QemuConsole >> *src, InputEvent *evt) >> } >> keycode = qcode_to_nextkbd_keycode[qcode]; >> - if (keycode == NEXTKBD_NO_KEY) { >> + if (!keycode) { >> return; >> } >> Thomas >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-08 13:24 ` BALATON Zoltan @ 2024-11-08 13:36 ` Thomas Huth 0 siblings, 0 replies; 14+ messages in thread From: Thomas Huth @ 2024-11-08 13:36 UTC (permalink / raw) To: BALATON Zoltan Cc: Philippe Mathieu-Daudé, Mark Cave-Ayland, peter.maydell, huth, berrange, qemu-devel On 08/11/2024 14.24, BALATON Zoltan wrote: > On Fri, 8 Nov 2024, BALATON Zoltan wrote: >> On Fri, 8 Nov 2024, Thomas Huth wrote: >>> On 06/11/2024 21.32, BALATON Zoltan wrote: >>>> On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: >>>>> On 6/11/24 13:00, BALATON Zoltan wrote: >>>>>> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>>>>>> Convert the next-kbd device from the legacy UI >>>>>>> qemu_add_kbd_event_handler() >>>>>>> function to use qemu_input_handler_register(). >>>>>>> >>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>>>>>> --- >>>>>>> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >>>>>>> 1 file changed, 108 insertions(+), 55 deletions(-) >>>>> >>>>> >>>>>>> -static const unsigned char next_keycodes[128] = { >>>>>>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>>>>>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>>>>>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>>>>>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>>>>>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>>>>>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>>>>>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>>>>>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>>>>>> +#define NEXTKBD_NO_KEY 0xff >>>>>> >>>>>> Now you don't need this 0xff define any more because you can use 0 as >>>>>> no key value then the [0 ... Q_KEY_CODE__MAX] init below can also be >>>>>> dropped because static variables are 0 init automatically. >>>>> >>>>> Whether 0 or 0xff is best for NO_KEY, I don't know. >>>>> However, definitions are useful when reviewing ... >>>>> >>>>>> >>>>>> Regards, >>>>>> BALATON Zoltan >>>>>> >>>>>>> +static const int qcode_to_nextkbd_keycode[] = { >>>>>>> + /* Make sure future additions are automatically set to >>>>>>> NEXTKBD_NO_KEY */ >>>>>>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>>>>>> + >>>>>>> + [Q_KEY_CODE_ESC] = 0x49, >>>>>>> + [Q_KEY_CODE_1] = 0x4a, >>>>>>> + [Q_KEY_CODE_2] = 0x4b, >>>>>>> + [Q_KEY_CODE_3] = 0x4c, >>>>>>> + [Q_KEY_CODE_4] = 0x4d, >>>>> [...] >>>>> >>>>>>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, >>>>>>> InputEvent *evt) >>>>>>> +{ >>>>>>> + NextKBDState *s = NEXTKBD(dev); >>>>>>> + int qcode, keycode; >>>>>>> + bool key_down = evt->u.key.data->down; >>>>>>> + >>>>>>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>>>>>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>>>>>> + return; >>>>>>> + } > > Never mind, there's already a check for that here so it should be OK. (Does > this still work with SHIFTs not having a value in the array? Maybe they > happen to be lower than the array size but maybe it's better to move this > check lower?) Q_KEY_CODE_SHIFT is one of the first entries in the enumeration, so we should be fine for now. I also tested the patch and the shift keys are working as expected. Thomas ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-08 13:13 ` BALATON Zoltan 2024-11-08 13:24 ` BALATON Zoltan @ 2024-11-08 15:45 ` Philippe Mathieu-Daudé 2024-11-08 19:57 ` BALATON Zoltan 1 sibling, 1 reply; 14+ messages in thread From: Philippe Mathieu-Daudé @ 2024-11-08 15:45 UTC (permalink / raw) To: BALATON Zoltan, Thomas Huth Cc: Mark Cave-Ayland, peter.maydell, huth, berrange, qemu-devel On 8/11/24 13:13, BALATON Zoltan wrote: > On Fri, 8 Nov 2024, Thomas Huth wrote: >> On 06/11/2024 21.32, BALATON Zoltan wrote: >>> On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: >>>> On 6/11/24 13:00, BALATON Zoltan wrote: >>>>> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>>>>> Convert the next-kbd device from the legacy UI >>>>>> qemu_add_kbd_event_handler() >>>>>> function to use qemu_input_handler_register(). >>>>>> >>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>>>>> --- >>>>>> hw/m68k/next-kbd.c | 163 >>>>>> ++++++++++++++++++++++++++++++--------------- >>>>>> 1 file changed, 108 insertions(+), 55 deletions(-) >>>> >>>> >>>>>> -static const unsigned char next_keycodes[128] = { >>>>>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>>>>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>>>>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>>>>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>>>>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>>>>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>>>>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>>>>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>>>>> +#define NEXTKBD_NO_KEY 0xff >>>>> >>>>> Now you don't need this 0xff define any more because you can use 0 >>>>> as no key value then the [0 ... Q_KEY_CODE__MAX] init below can >>>>> also be dropped because static variables are 0 init automatically. >>>> >>>> Whether 0 or 0xff is best for NO_KEY, I don't know. >>>> However, definitions are useful when reviewing ... >>>> >>>>> >>>>> Regards, >>>>> BALATON Zoltan >>>>> >>>>>> +static const int qcode_to_nextkbd_keycode[] = { >>>>>> + /* Make sure future additions are automatically set to >>>>>> NEXTKBD_NO_KEY */ >>>>>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>>>>> + >>>>>> + [Q_KEY_CODE_ESC] = 0x49, >>>>>> + [Q_KEY_CODE_1] = 0x4a, >>>>>> + [Q_KEY_CODE_2] = 0x4b, >>>>>> + [Q_KEY_CODE_3] = 0x4c, >>>>>> + [Q_KEY_CODE_4] = 0x4d, >>>> [...] >>>> >>>>>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, >>>>>> InputEvent *evt) >>>>>> +{ >>>>>> + NextKBDState *s = NEXTKBD(dev); >>>>>> + int qcode, keycode; >>>>>> + bool key_down = evt->u.key.data->down; >>>>>> + >>>>>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>>>>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + /* Shift key currently has no keycode, so handle separately */ >>>>>> + if (qcode == Q_KEY_CODE_SHIFT) { >>>>>> + if (key_down) { >>>>>> + s->shift |= KD_LSHIFT; >>>>>> + } else { >>>>>> + s->shift &= ~KD_LSHIFT; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (qcode == Q_KEY_CODE_SHIFT_R) { >>>>>> + if (key_down) { >>>>>> + s->shift |= KD_RSHIFT; >>>>>> + } else { >>>>>> + s->shift &= ~KD_RSHIFT; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + keycode = qcode_to_nextkbd_keycode[qcode]; >>>>>> + if (keycode == NEXTKBD_NO_KEY) { >>>> >>>> ... here ^ >>> >>> I this case !keycode is pretty self explanatory IMO. >> >> Ok, I'll pick up the patch with this change added on top: >> >> diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c >> --- a/hw/m68k/next-kbd.c >> +++ b/hw/m68k/next-kbd.c >> @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = { >> .endianness = DEVICE_NATIVE_ENDIAN, >> }; >> -#define NEXTKBD_NO_KEY 0xff >> - >> static const int qcode_to_nextkbd_keycode[] = { >> - /* Make sure future additions are automatically set to >> NEXTKBD_NO_KEY */ >> - [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, > > Thinking about it more, removing this may make the array smaller so we'd > either need some max value define (or get it something like > qcode_to_nextkbd_keycode[ARRAY_SIZE(qcode_to_nextkbd_keycode) - 1] or > so) and check if qcode is not > than that or declare the array as > [Q_KEY_CODE__MAX] to make sure we're not trying to access values after > the end. Maybe it's simplest to do > qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] as this is not much wasted > space, unless this can't overflow for some other reason I don't know about. Agreed, qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] is future-proof. > > Regards, > BALATON Zoltan > >> - >> [Q_KEY_CODE_ESC] = 0x49, >> [Q_KEY_CODE_1] = 0x4a, >> [Q_KEY_CODE_2] = 0x4b, >> @@ -276,7 +271,7 @@ static void nextkbd_event(DeviceState *dev, >> QemuConsole *src, InputEvent *evt) >> } >> keycode = qcode_to_nextkbd_keycode[qcode]; >> - if (keycode == NEXTKBD_NO_KEY) { >> + if (!keycode) { >> return; >> } >> Thomas >> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register() 2024-11-08 15:45 ` Philippe Mathieu-Daudé @ 2024-11-08 19:57 ` BALATON Zoltan 0 siblings, 0 replies; 14+ messages in thread From: BALATON Zoltan @ 2024-11-08 19:57 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Thomas Huth, Mark Cave-Ayland, peter.maydell, huth, berrange, qemu-devel [-- Attachment #1: Type: text/plain, Size: 5293 bytes --] On Fri, 8 Nov 2024, Philippe Mathieu-Daudé wrote: > On 8/11/24 13:13, BALATON Zoltan wrote: >> On Fri, 8 Nov 2024, Thomas Huth wrote: >>> On 06/11/2024 21.32, BALATON Zoltan wrote: >>>> On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote: >>>>> On 6/11/24 13:00, BALATON Zoltan wrote: >>>>>> On Wed, 6 Nov 2024, Mark Cave-Ayland wrote: >>>>>>> Convert the next-kbd device from the legacy UI >>>>>>> qemu_add_kbd_event_handler() >>>>>>> function to use qemu_input_handler_register(). >>>>>>> >>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>>>> Reviewed-by: Thomas Huth <huth@tuxfamily.org> >>>>>>> --- >>>>>>> hw/m68k/next-kbd.c | 163 ++++++++++++++++++++++++++++++--------------- >>>>>>> 1 file changed, 108 insertions(+), 55 deletions(-) >>>>> >>>>> >>>>>>> -static const unsigned char next_keycodes[128] = { >>>>>>> - 0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F, >>>>>>> - 0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00, >>>>>>> - 0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06, >>>>>>> - 0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A, >>>>>>> - 0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C, >>>>>>> - 0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34, >>>>>>> - 0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00, >>>>>>> - 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, >>>>>>> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 >>>>>>> +#define NEXTKBD_NO_KEY 0xff >>>>>> >>>>>> Now you don't need this 0xff define any more because you can use 0 as >>>>>> no key value then the [0 ... Q_KEY_CODE__MAX] init below can also be >>>>>> dropped because static variables are 0 init automatically. >>>>> >>>>> Whether 0 or 0xff is best for NO_KEY, I don't know. >>>>> However, definitions are useful when reviewing ... >>>>> >>>>>> >>>>>> Regards, >>>>>> BALATON Zoltan >>>>>> >>>>>>> +static const int qcode_to_nextkbd_keycode[] = { >>>>>>> + /* Make sure future additions are automatically set to >>>>>>> NEXTKBD_NO_KEY */ >>>>>>> + [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >>>>>>> + >>>>>>> + [Q_KEY_CODE_ESC] = 0x49, >>>>>>> + [Q_KEY_CODE_1] = 0x4a, >>>>>>> + [Q_KEY_CODE_2] = 0x4b, >>>>>>> + [Q_KEY_CODE_3] = 0x4c, >>>>>>> + [Q_KEY_CODE_4] = 0x4d, >>>>> [...] >>>>> >>>>>>> +static void nextkbd_event(DeviceState *dev, QemuConsole *src, >>>>>>> InputEvent *evt) >>>>>>> +{ >>>>>>> + NextKBDState *s = NEXTKBD(dev); >>>>>>> + int qcode, keycode; >>>>>>> + bool key_down = evt->u.key.data->down; >>>>>>> + >>>>>>> + qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key); >>>>>>> + if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) { >>>>>>> + return; >>>>>>> + } >>>>>>> + >>>>>>> + /* Shift key currently has no keycode, so handle separately */ >>>>>>> + if (qcode == Q_KEY_CODE_SHIFT) { >>>>>>> + if (key_down) { >>>>>>> + s->shift |= KD_LSHIFT; >>>>>>> + } else { >>>>>>> + s->shift &= ~KD_LSHIFT; >>>>>>> + } >>>>>>> + } >>>>>>> + >>>>>>> + if (qcode == Q_KEY_CODE_SHIFT_R) { >>>>>>> + if (key_down) { >>>>>>> + s->shift |= KD_RSHIFT; >>>>>>> + } else { >>>>>>> + s->shift &= ~KD_RSHIFT; >>>>>>> + } >>>>>>> + } >>>>>>> + >>>>>>> + keycode = qcode_to_nextkbd_keycode[qcode]; >>>>>>> + if (keycode == NEXTKBD_NO_KEY) { >>>>> >>>>> ... here ^ >>>> >>>> I this case !keycode is pretty self explanatory IMO. >>> >>> Ok, I'll pick up the patch with this change added on top: >>> >>> diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c >>> --- a/hw/m68k/next-kbd.c >>> +++ b/hw/m68k/next-kbd.c >>> @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = { >>> .endianness = DEVICE_NATIVE_ENDIAN, >>> }; >>> -#define NEXTKBD_NO_KEY 0xff >>> - >>> static const int qcode_to_nextkbd_keycode[] = { >>> - /* Make sure future additions are automatically set to NEXTKBD_NO_KEY >>> */ >>> - [0 ... Q_KEY_CODE__MAX] = NEXTKBD_NO_KEY, >> >> Thinking about it more, removing this may make the array smaller so we'd >> either need some max value define (or get it something like >> qcode_to_nextkbd_keycode[ARRAY_SIZE(qcode_to_nextkbd_keycode) - 1] or so) >> and check if qcode is not > than that or declare the array as >> [Q_KEY_CODE__MAX] to make sure we're not trying to access values after the >> end. Maybe it's simplest to do qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] >> as this is not much wasted space, unless this can't overflow for some other >> reason I don't know about. > > Agreed, qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] is future-proof. Actually there's already a check for array size where it's used but I've only noticed that after writing this so it should be OK without enlarging the array. Regards, BALATON Zoltan ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] ui/input-legacy.c: remove unused legacy qemu_add_kbd_event_handler() function 2024-11-06 12:09 [PATCH v4 0/2] next-kbd: convert to use qemu_input_handler_register() Mark Cave-Ayland 2024-11-06 12:09 ` [PATCH v4 1/2] " Mark Cave-Ayland @ 2024-11-06 12:09 ` Mark Cave-Ayland 1 sibling, 0 replies; 14+ messages in thread From: Mark Cave-Ayland @ 2024-11-06 12:09 UTC (permalink / raw) To: peter.maydell, huth, berrange, qemu-devel Since the last keyboard device has now been converted over to use qemu_input_handler_register(), the legacy qemu_add_kbd_event_handler() function is now unused and can be removed. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/ui/console.h | 2 -- ui/input-legacy.c | 37 ------------------------------------- 2 files changed, 39 deletions(-) diff --git a/include/ui/console.h b/include/ui/console.h index 5832d52a8a..46b3128185 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -70,8 +70,6 @@ typedef struct QEMUPutMouseEntry QEMUPutMouseEntry; typedef struct QEMUPutKbdEntry QEMUPutKbdEntry; typedef struct QEMUPutLEDEntry QEMUPutLEDEntry; -QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, - void *opaque); QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func, void *opaque, int absolute, const char *name); diff --git a/ui/input-legacy.c b/ui/input-legacy.c index 210ae5eaca..ca4bccb411 100644 --- a/ui/input-legacy.c +++ b/ui/input-legacy.c @@ -109,43 +109,6 @@ void qmp_send_key(KeyValueList *keys, bool has_hold_time, int64_t hold_time, g_free(up); } -static void legacy_kbd_event(DeviceState *dev, QemuConsole *src, - InputEvent *evt) -{ - QEMUPutKbdEntry *entry = (QEMUPutKbdEntry *)dev; - int scancodes[3], i, count; - InputKeyEvent *key = evt->u.key.data; - - if (!entry || !entry->put_kbd) { - return; - } - count = qemu_input_key_value_to_scancode(key->key, - key->down, - scancodes); - for (i = 0; i < count; i++) { - entry->put_kbd(entry->opaque, scancodes[i]); - } -} - -static const QemuInputHandler legacy_kbd_handler = { - .name = "legacy-kbd", - .mask = INPUT_EVENT_MASK_KEY, - .event = legacy_kbd_event, -}; - -QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque) -{ - QEMUPutKbdEntry *entry; - - entry = g_new0(QEMUPutKbdEntry, 1); - entry->put_kbd = func; - entry->opaque = opaque; - entry->s = qemu_input_handler_register((DeviceState *)entry, - &legacy_kbd_handler); - qemu_input_handler_activate(entry->s); - return entry; -} - static void legacy_mouse_event(DeviceState *dev, QemuConsole *src, InputEvent *evt) { -- 2.39.5 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-11-08 19:58 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-06 12:09 [PATCH v4 0/2] next-kbd: convert to use qemu_input_handler_register() Mark Cave-Ayland 2024-11-06 12:09 ` [PATCH v4 1/2] " Mark Cave-Ayland 2024-11-06 12:21 ` Daniel P. Berrangé 2024-11-06 13:00 ` BALATON Zoltan 2024-11-06 15:50 ` Philippe Mathieu-Daudé 2024-11-06 20:32 ` BALATON Zoltan 2024-11-08 10:05 ` Thomas Huth 2024-11-08 12:26 ` Mark Cave-Ayland 2024-11-08 13:13 ` BALATON Zoltan 2024-11-08 13:24 ` BALATON Zoltan 2024-11-08 13:36 ` Thomas Huth 2024-11-08 15:45 ` Philippe Mathieu-Daudé 2024-11-08 19:57 ` BALATON Zoltan 2024-11-06 12:09 ` [PATCH v4 2/2] ui/input-legacy.c: remove unused legacy qemu_add_kbd_event_handler() function Mark Cave-Ayland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).