* [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
* [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
* 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
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).