[PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev

[PATCH v2 0/6] arm/stellaris: convert gamepad input device to qdev

[Peter Maydell]

This patchseries converts the stellaris board's gamepad input device
to qdev. This isn't a very important bit of conversion (I was just
looking for a small tail-end-of-the-week task), but it does reduce by
one the number of users of a couple of legacy APIs: vmstate_register()
and qemu_add_kbd_event_handler().

All patches reviewed and tested; changes v1->v2 are minor only:
 * bump migration version number and mention migration break
   in commit message
 * drop unneeded private/public comment lines
 * make QemuInputHandler struct const

I've included Kevin's qdev_prop_set_array() patch here, and will
take this via target-arm.next if it doesn't get in via another
route first.

thanks
-- PMM

Kevin Wolf (1):
  qdev: Add qdev_prop_set_array()

Peter Maydell (5):
  hw/input/stellaris_input: Rename to stellaris_gamepad
  hw/input/stellaris_gamepad: Rename structs to our usual convention
  hw/input/stellaris_gamepad: Remove StellarisGamepadButton struct
  hw/input/stellaris_input: Convert to qdev
  hw/input/stellaris_gamepad: Convert to qemu_input_handler_register()

 include/hw/input/gamepad.h           |  18 -----
 include/hw/input/stellaris_gamepad.h |  36 ++++++++++
 include/hw/qdev-properties.h         |   3 +
 hw/arm/stellaris.c                   |  34 ++++++---
 hw/core/qdev-properties.c            |  21 ++++++
 hw/input/stellaris_gamepad.c         | 102 +++++++++++++++++++++++++++
 hw/input/stellaris_input.c           |  93 ------------------------
 hw/arm/Kconfig                       |   2 +-
 hw/input/Kconfig                     |   2 +-
 hw/input/meson.build                 |   2 +-
 10 files changed, 190 insertions(+), 123 deletions(-)
 delete mode 100644 include/hw/input/gamepad.h
 create mode 100644 include/hw/input/stellaris_gamepad.h
 create mode 100644 hw/input/stellaris_gamepad.c
 delete mode 100644 hw/input/stellaris_input.c

-- 
2.34.1

[PATCH v2 1/6] hw/input/stellaris_input: Rename to stellaris_gamepad

[Peter Maydell]

This source file implements a stellaris gamepad device; rename
it so that it is a closer match to the device name.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/input/{gamepad.h => stellaris_gamepad.h} | 6 +++---
 hw/arm/stellaris.c                                  | 2 +-
 hw/input/{stellaris_input.c => stellaris_gamepad.c} | 2 +-
 hw/arm/Kconfig                                      | 2 +-
 hw/input/Kconfig                                    | 2 +-
 hw/input/meson.build                                | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)
 rename include/hw/input/{gamepad.h => stellaris_gamepad.h} (77%)
 rename hw/input/{stellaris_input.c => stellaris_gamepad.c} (98%)

diff --git a/include/hw/input/gamepad.h b/include/hw/input/stellaris_gamepad.h
similarity index 77%
rename from include/hw/input/gamepad.h
rename to include/hw/input/stellaris_gamepad.h
index 6f6aa2406aa..23cfd3c95f3 100644
--- a/include/hw/input/gamepad.h
+++ b/include/hw/input/stellaris_gamepad.h
@@ -8,11 +8,11 @@
  * See the COPYING file in the top-level directory.
  */
 
-#ifndef HW_INPUT_GAMEPAD_H
-#define HW_INPUT_GAMEPAD_H
+#ifndef HW_INPUT_STELLARIS_GAMEPAD_H
+#define HW_INPUT_STELLARIS_GAMEPAD_H
 
 
-/* stellaris_input.c */
+/* stellaris_gamepad.c */
 void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
 
 #endif
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index aa5b0ddfaa5..96585dd7106 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -23,7 +23,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/arm/armv7m.h"
 #include "hw/char/pl011.h"
-#include "hw/input/gamepad.h"
+#include "hw/input/stellaris_gamepad.h"
 #include "hw/irq.h"
 #include "hw/watchdog/cmsdk-apb-watchdog.h"
 #include "migration/vmstate.h"
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_gamepad.c
similarity index 98%
rename from hw/input/stellaris_input.c
rename to hw/input/stellaris_gamepad.c
index e6ee5e11f1b..3bab557cab3 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_gamepad.c
@@ -8,7 +8,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/input/gamepad.h"
+#include "hw/input/stellaris_gamepad.h"
 #include "hw/irq.h"
 #include "migration/vmstate.h"
 #include "ui/console.h"
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 7e683484405..841f3131ea5 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -291,7 +291,7 @@ config STELLARIS
     select SSD0303 # OLED display
     select SSD0323 # OLED display
     select SSI_SD
-    select STELLARIS_INPUT
+    select STELLARIS_GAMEPAD
     select STELLARIS_ENET # ethernet
     select STELLARIS_GPTM # general purpose timer module
     select UNIMP
diff --git a/hw/input/Kconfig b/hw/input/Kconfig
index 55865bb3869..f86e98c8293 100644
--- a/hw/input/Kconfig
+++ b/hw/input/Kconfig
@@ -20,7 +20,7 @@ config PL050
 config PS2
     bool
 
-config STELLARIS_INPUT
+config STELLARIS_GAMEPAD
     bool
 
 config TSC2005
diff --git a/hw/input/meson.build b/hw/input/meson.build
index c0d44821800..640556bbbcc 100644
--- a/hw/input/meson.build
+++ b/hw/input/meson.build
@@ -5,7 +5,7 @@ system_ss.add(when: 'CONFIG_LM832X', if_true: files('lm832x.c'))
 system_ss.add(when: 'CONFIG_PCKBD', if_true: files('pckbd.c'))
 system_ss.add(when: 'CONFIG_PL050', if_true: files('pl050.c'))
 system_ss.add(when: 'CONFIG_PS2', if_true: files('ps2.c'))
-system_ss.add(when: 'CONFIG_STELLARIS_INPUT', if_true: files('stellaris_input.c'))
+system_ss.add(when: 'CONFIG_STELLARIS_GAMEPAD', if_true: files('stellaris_gamepad.c'))
 system_ss.add(when: 'CONFIG_TSC2005', if_true: files('tsc2005.c'))
 
 system_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true: files('virtio-input.c'))
-- 
2.34.1

[PATCH v2 2/6] hw/input/stellaris_gamepad: Rename structs to our usual convention

[Peter Maydell]

Rename the structs in stellaris_gamepad.c to our now-standard
CamelCase convention.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/input/stellaris_gamepad.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 3bab557cab3..377101a4035 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -17,17 +17,17 @@ typedef struct {
     qemu_irq irq;
     int keycode;
     uint8_t pressed;
-} gamepad_button;
+} StellarisGamepadButton;
 
 typedef struct {
-    gamepad_button *buttons;
+    StellarisGamepadButton *buttons;
     int num_buttons;
     int extension;
-} gamepad_state;
+} StellarisGamepad;
 
 static void stellaris_gamepad_put_key(void * opaque, int keycode)
 {
-    gamepad_state *s = (gamepad_state *)opaque;
+    StellarisGamepad *s = (StellarisGamepad *)opaque;
     int i;
     int down;
 
@@ -55,7 +55,7 @@ static const VMStateDescription vmstate_stellaris_button = {
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT8(pressed, gamepad_button),
+        VMSTATE_UINT8(pressed, StellarisGamepadButton),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -65,11 +65,11 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
     .version_id = 2,
     .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_INT32(extension, gamepad_state),
-        VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state,
+        VMSTATE_INT32(extension, StellarisGamepad),
+        VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, StellarisGamepad,
                                             num_buttons,
                                             vmstate_stellaris_button,
-                                            gamepad_button),
+                                            StellarisGamepadButton),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -77,11 +77,11 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
 /* Returns an array of 5 output slots.  */
 void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
 {
-    gamepad_state *s;
+    StellarisGamepad *s;
     int i;
 
-    s = g_new0(gamepad_state, 1);
-    s->buttons = g_new0(gamepad_button, n);
+    s = g_new0(StellarisGamepad, 1);
+    s->buttons = g_new0(StellarisGamepadButton, n);
     for (i = 0; i < n; i++) {
         s->buttons[i].irq = irq[i];
         s->buttons[i].keycode = keycode[i];
-- 
2.34.1

[PATCH v2 3/6] qdev: Add qdev_prop_set_array()

[Peter Maydell]

From: Kevin Wolf <kwolf@redhat.com>

Instead of exposing the ugly hack of how we represent arrays in qdev (a
static "foo-len" property and after it is set, dynamically created
"foo[i]" properties) to boards, add an interface that allows setting the
whole array at once.

Once all internal users of devices with array properties have been
converted to use this function, we can change the implementation to move
away from this hack.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/qdev-properties.h |  3 +++
 hw/core/qdev-properties.c    | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index e1df08876c6..7fa2fdb7c94 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -206,6 +206,9 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name,
                            const uint8_t *value);
 void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
 
+/* Takes ownership of @values */
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values);
+
 void *object_field_prop_ptr(Object *obj, Property *prop);
 
 void qdev_prop_register_global(GlobalProperty *prop);
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 357b8761b54..950ef48e013 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -3,12 +3,14 @@
 #include "qapi/error.h"
 #include "qapi/qapi-types-misc.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qlist.h"
 #include "qemu/ctype.h"
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
 #include "qemu/units.h"
 #include "qemu/cutils.h"
 #include "qdev-prop-internal.h"
+#include "qom/qom-qobject.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
@@ -739,6 +741,25 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
                             &error_abort);
 }
 
+void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
+{
+    const QListEntry *entry;
+    g_autofree char *prop_len = g_strdup_printf("len-%s", name);
+    uint32_t i = 0;
+
+    object_property_set_int(OBJECT(dev), prop_len, qlist_size(values),
+                            &error_abort);
+
+    QLIST_FOREACH_ENTRY(values, entry) {
+        g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i);
+        object_property_set_qobject(OBJECT(dev), prop_idx, entry->value,
+                                    &error_abort);
+        i++;
+    }
+
+    qobject_unref(values);
+}
+
 static GPtrArray *global_props(void)
 {
     static GPtrArray *gp;
-- 
2.34.1

[PATCH v2 4/6] hw/input/stellaris_gamepad: Remove StellarisGamepadButton struct

[Peter Maydell]

Currently for each button on the device we have a
StellarisGamepadButton struct which has the irq, keycode and pressed
state for it.  When we convert to qdev, the qdev property and GPIO
APIs are going to require that we have separate arrays for the irqs
and keycodes.  Convert from array-of-structs to three separate arrays
in preparation.

This is a migration compatibility break for the stellaris boards
(lm3s6965evb, lm3s811evb).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
--
v1=>v2: mention migration compat break in commit message;
  bump version fields in vmstate
---
 hw/input/stellaris_gamepad.c | 47 ++++++++++++------------------------
 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 377101a4035..82ddc47a26d 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -14,15 +14,11 @@
 #include "ui/console.h"
 
 typedef struct {
-    qemu_irq irq;
-    int keycode;
-    uint8_t pressed;
-} StellarisGamepadButton;
-
-typedef struct {
-    StellarisGamepadButton *buttons;
-    int num_buttons;
+    uint32_t num_buttons;
     int extension;
+    qemu_irq *irqs;
+    uint32_t *keycodes;
+    uint8_t *pressed;
 } StellarisGamepad;
 
 static void stellaris_gamepad_put_key(void * opaque, int keycode)
@@ -40,36 +36,23 @@ static void stellaris_gamepad_put_key(void * opaque, int keycode)
     keycode = (keycode & 0x7f) | s->extension;
 
     for (i = 0; i < s->num_buttons; i++) {
-        if (s->buttons[i].keycode == keycode
-                && s->buttons[i].pressed != down) {
-            s->buttons[i].pressed = down;
-            qemu_set_irq(s->buttons[i].irq, down);
+        if (s->keycodes[i] == keycode && s->pressed[i] != down) {
+            s->pressed[i] = down;
+            qemu_set_irq(s->irqs[i], down);
         }
     }
 
     s->extension = 0;
 }
 
-static const VMStateDescription vmstate_stellaris_button = {
-    .name = "stellaris_button",
-    .version_id = 0,
-    .minimum_version_id = 0,
-    .fields = (VMStateField[]) {
-        VMSTATE_UINT8(pressed, StellarisGamepadButton),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
 static const VMStateDescription vmstate_stellaris_gamepad = {
     .name = "stellaris_gamepad",
-    .version_id = 2,
-    .minimum_version_id = 2,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_INT32(extension, StellarisGamepad),
-        VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, StellarisGamepad,
-                                            num_buttons,
-                                            vmstate_stellaris_button,
-                                            StellarisGamepadButton),
+        VMSTATE_VARRAY_UINT32(pressed, StellarisGamepad, num_buttons,
+                              0, vmstate_info_uint8, uint8_t),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -81,10 +64,12 @@ void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
     int i;
 
     s = g_new0(StellarisGamepad, 1);
-    s->buttons = g_new0(StellarisGamepadButton, n);
+    s->irqs = g_new0(qemu_irq, n);
+    s->keycodes = g_new0(uint32_t, n);
+    s->pressed = g_new0(uint8_t, n);
     for (i = 0; i < n; i++) {
-        s->buttons[i].irq = irq[i];
-        s->buttons[i].keycode = keycode[i];
+        s->irqs[i] = irq[i];
+        s->keycodes[i] = keycode[i];
     }
     s->num_buttons = n;
     qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
-- 
2.34.1

[PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Peter Maydell]

Convert the hw/input/stellaris_input device to qdev.

The interface uses an array property for the board to specify the
keycodes to use, so the s->keycodes memory is now allocated by the
array-property machinery.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1->v2: drop private/public comment lines
---
 include/hw/input/stellaris_gamepad.h | 22 ++++++++-
 hw/arm/stellaris.c                   | 26 +++++++---
 hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
 3 files changed, 89 insertions(+), 32 deletions(-)

diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
index 23cfd3c95f3..6140b889a28 100644
--- a/include/hw/input/stellaris_gamepad.h
+++ b/include/hw/input/stellaris_gamepad.h
@@ -11,8 +11,26 @@
 #ifndef HW_INPUT_STELLARIS_GAMEPAD_H
 #define HW_INPUT_STELLARIS_GAMEPAD_H
 
+#include "hw/sysbus.h"
+#include "qom/object.h"
 
-/* stellaris_gamepad.c */
-void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
+/*
+ * QEMU interface:
+ *  + QOM array property "keycodes": uint32_t QEMU keycodes to handle
+ *  + unnamed GPIO outputs: one per keycode, in the same order as the
+ *    "keycodes" array property entries; asserted when key is down
+ */
+
+#define TYPE_STELLARIS_GAMEPAD "stellaris-gamepad"
+OBJECT_DECLARE_SIMPLE_TYPE(StellarisGamepad, STELLARIS_GAMEPAD)
+
+struct StellarisGamepad {
+    SysBusDevice parent_obj;
+    uint32_t num_buttons;
+    qemu_irq *irqs;
+    uint32_t *keycodes;
+    uint8_t *pressed;
+    int extension;
+};
 
 #endif
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 96585dd7106..707b0dae375 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -31,6 +31,7 @@
 #include "hw/timer/stellaris-gptm.h"
 #include "hw/qdev-clock.h"
 #include "qom/object.h"
+#include "qapi/qmp/qlist.h"
 
 #define GPIO_A 0
 #define GPIO_B 1
@@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
         sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
     }
     if (board->peripherals & BP_GAMEPAD) {
-        qemu_irq gpad_irq[5];
+        QList *gpad_keycode_list = qlist_new();
         static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
+        DeviceState *gpad;
 
-        gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
-        gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
-        gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
-        gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
-        gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
+        gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
+        for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
+            qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
+        }
+        qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
 
-        stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
+        qdev_connect_gpio_out(gpad, 0,
+                              qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
+        qdev_connect_gpio_out(gpad, 1,
+                              qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
+        qdev_connect_gpio_out(gpad, 2,
+                              qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
+        qdev_connect_gpio_out(gpad, 3,
+                              qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
+        qdev_connect_gpio_out(gpad, 4,
+                              qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
     }
     for (i = 0; i < 7; i++) {
         if (board->dc4 & (1 << i)) {
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 82ddc47a26d..6ccf0e80adc 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -8,19 +8,13 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/input/stellaris_gamepad.h"
 #include "hw/irq.h"
+#include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "ui/console.h"
 
-typedef struct {
-    uint32_t num_buttons;
-    int extension;
-    qemu_irq *irqs;
-    uint32_t *keycodes;
-    uint8_t *pressed;
-} StellarisGamepad;
-
 static void stellaris_gamepad_put_key(void * opaque, int keycode)
 {
     StellarisGamepad *s = (StellarisGamepad *)opaque;
@@ -57,22 +51,55 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
     }
 };
 
-/* Returns an array of 5 output slots.  */
-void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
+static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
 {
-    StellarisGamepad *s;
-    int i;
+    StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
 
-    s = g_new0(StellarisGamepad, 1);
-    s->irqs = g_new0(qemu_irq, n);
-    s->keycodes = g_new0(uint32_t, n);
-    s->pressed = g_new0(uint8_t, n);
-    for (i = 0; i < n; i++) {
-        s->irqs[i] = irq[i];
-        s->keycodes[i] = keycode[i];
+    if (s->num_buttons == 0) {
+        error_setg(errp, "keycodes property array must be set");
+        return;
     }
-    s->num_buttons = n;
-    qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
-                     &vmstate_stellaris_gamepad, s);
+
+    s->irqs = g_new0(qemu_irq, s->num_buttons);
+    s->pressed = g_new0(uint8_t, s->num_buttons);
+    qdev_init_gpio_out(dev, s->irqs, s->num_buttons);
+    qemu_add_kbd_event_handler(stellaris_gamepad_put_key, dev);
 }
+
+static void stellaris_gamepad_reset_enter(Object *obj, ResetType type)
+{
+    StellarisGamepad *s = STELLARIS_GAMEPAD(obj);
+
+    memset(s->pressed, 0, s->num_buttons * sizeof(uint8_t));
+}
+
+static Property stellaris_gamepad_properties[] = {
+    DEFINE_PROP_ARRAY("keycodes", StellarisGamepad, num_buttons,
+                      keycodes, qdev_prop_uint32, uint32_t),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void stellaris_gamepad_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+    rc->phases.enter = stellaris_gamepad_reset_enter;
+    dc->realize = stellaris_gamepad_realize;
+    dc->vmsd = &vmstate_stellaris_gamepad;
+    device_class_set_props(dc, stellaris_gamepad_properties);
+}
+
+static const TypeInfo stellaris_gamepad_info = {
+    .name = TYPE_STELLARIS_GAMEPAD,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(StellarisGamepad),
+    .class_init = stellaris_gamepad_class_init,
+};
+
+static void stellaris_gamepad_register_types(void)
+{
+    type_register_static(&stellaris_gamepad_info);
+}
+
+type_init(stellaris_gamepad_register_types);
-- 
2.34.1

[PATCH v2 6/6] hw/input/stellaris_gamepad: Convert to qemu_input_handler_register()

[Peter Maydell]

Now that we have converted to qdev, we can use the newer
qemu_input_handler_register() API rather than the legacy
qemu_add_kbd_event_handler().

Since we only have one user, take the opportunity to convert
from scancodes to QCodes, rather than using
qemu_input_key_value_to_scancode() (which adds an 0xe0
prefix and encodes up/down indication in the scancode,
which our old handler function then had to reverse). That
lets us drop the old state field which was tracking whether
we were halfway through a two-byte scancode.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1->v2: bump vmstate version number again
        mark QemuInputHandler struct as const
---
 include/hw/input/stellaris_gamepad.h |  2 +-
 hw/arm/stellaris.c                   |  6 ++++-
 hw/input/stellaris_gamepad.c         | 37 +++++++++++++---------------
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
index 6140b889a28..b0c66a35531 100644
--- a/include/hw/input/stellaris_gamepad.h
+++ b/include/hw/input/stellaris_gamepad.h
@@ -17,6 +17,7 @@
 /*
  * QEMU interface:
  *  + QOM array property "keycodes": uint32_t QEMU keycodes to handle
+ *    (these are QCodes, ie the Q_KEY_* values)
  *  + unnamed GPIO outputs: one per keycode, in the same order as the
  *    "keycodes" array property entries; asserted when key is down
  */
@@ -30,7 +31,6 @@ struct StellarisGamepad {
     qemu_irq *irqs;
     uint32_t *keycodes;
     uint8_t *pressed;
-    int extension;
 };
 
 #endif
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 707b0dae375..dd90f686bfa 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -32,6 +32,7 @@
 #include "hw/qdev-clock.h"
 #include "qom/object.h"
 #include "qapi/qmp/qlist.h"
+#include "ui/input.h"
 
 #define GPIO_A 0
 #define GPIO_B 1
@@ -1276,7 +1277,10 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     }
     if (board->peripherals & BP_GAMEPAD) {
         QList *gpad_keycode_list = qlist_new();
-        static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
+        static const int gpad_keycode[5] = {
+            Q_KEY_CODE_UP, Q_KEY_CODE_DOWN, Q_KEY_CODE_LEFT,
+            Q_KEY_CODE_RIGHT, Q_KEY_CODE_CTRL,
+        };
         DeviceState *gpad;
 
         gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 6ccf0e80adc..3533becff5d 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -15,42 +15,39 @@
 #include "migration/vmstate.h"
 #include "ui/console.h"
 
-static void stellaris_gamepad_put_key(void * opaque, int keycode)
+static void stellaris_gamepad_event(DeviceState *dev, QemuConsole *src,
+                                    InputEvent *evt)
 {
-    StellarisGamepad *s = (StellarisGamepad *)opaque;
+    StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
+    InputKeyEvent *key = evt->u.key.data;
+    int qcode = qemu_input_key_value_to_qcode(key->key);
     int i;
-    int down;
-
-    if (keycode == 0xe0 && !s->extension) {
-        s->extension = 0x80;
-        return;
-    }
-
-    down = (keycode & 0x80) == 0;
-    keycode = (keycode & 0x7f) | s->extension;
 
     for (i = 0; i < s->num_buttons; i++) {
-        if (s->keycodes[i] == keycode && s->pressed[i] != down) {
-            s->pressed[i] = down;
-            qemu_set_irq(s->irqs[i], down);
+        if (s->keycodes[i] == qcode && s->pressed[i] != key->down) {
+            s->pressed[i] = key->down;
+            qemu_set_irq(s->irqs[i], key->down);
         }
     }
-
-    s->extension = 0;
 }
 
 static const VMStateDescription vmstate_stellaris_gamepad = {
     .name = "stellaris_gamepad",
-    .version_id = 3,
-    .minimum_version_id = 3,
+    .version_id = 4,
+    .minimum_version_id = 4,
     .fields = (VMStateField[]) {
-        VMSTATE_INT32(extension, StellarisGamepad),
         VMSTATE_VARRAY_UINT32(pressed, StellarisGamepad, num_buttons,
                               0, vmstate_info_uint8, uint8_t),
         VMSTATE_END_OF_LIST()
     }
 };
 
+static const QemuInputHandler stellaris_gamepad_handler = {
+    .name = "Stellaris Gamepad",
+    .mask = INPUT_EVENT_MASK_KEY,
+    .event = stellaris_gamepad_event,
+};
+
 static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
 {
     StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
@@ -63,7 +60,7 @@ static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
     s->irqs = g_new0(qemu_irq, s->num_buttons);
     s->pressed = g_new0(uint8_t, s->num_buttons);
     qdev_init_gpio_out(dev, s->irqs, s->num_buttons);
-    qemu_add_kbd_event_handler(stellaris_gamepad_put_key, dev);
+    qemu_input_handler_register(dev, &stellaris_gamepad_handler);
 }
 
 static void stellaris_gamepad_reset_enter(Object *obj, ResetType type)
-- 
2.34.1

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Philippe Mathieu-Daudé]

Hi Peter,

Cc'ing Markus for QObject.

On 30/10/23 12:48, Peter Maydell wrote:
> Convert the hw/input/stellaris_input device to qdev.
> 
> The interface uses an array property for the board to specify the
> keycodes to use, so the s->keycodes memory is now allocated by the
> array-property machinery.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> v1->v2: drop private/public comment lines
> ---
>   include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>   hw/arm/stellaris.c                   | 26 +++++++---
>   hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
>   3 files changed, 89 insertions(+), 32 deletions(-)


> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 96585dd7106..707b0dae375 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -31,6 +31,7 @@
>   #include "hw/timer/stellaris-gptm.h"
>   #include "hw/qdev-clock.h"
>   #include "qom/object.h"
> +#include "qapi/qmp/qlist.h"
>   
>   #define GPIO_A 0
>   #define GPIO_B 1
> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>           sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>       }
>       if (board->peripherals & BP_GAMEPAD) {
> -        qemu_irq gpad_irq[5];
> +        QList *gpad_keycode_list = qlist_new();

I'm trying to understand better qlist_new(), but unfortunately there
is not much documentation. Looking at how the allocated list was
released, I found use of g_autoptr in tests/unit/check-qobject.c,
so I tried:

            g_autoptr(QList) gpad_keycode_list = qlist_new();

But QEMU crashes elsewhere which seems unrelated:

* thread #2, stop reason = signal SIGABRT
   * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
     frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
     frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
     frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
     frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
     frame #5: 0x8b05b094 
libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
     frame #6: 0x8b05a2a8 
libsystem_malloc.dylib`nanov2_allocate_outlined + 404
     frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
     frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage 
+ 76
     frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
     frame #10: 0x003a9920 qemu-system-ppc`object_unref [inlined] 
object_property_del_all(obj=0x42023e00) at object.c:635:34
     frame #11: 0x003a9914 qemu-system-ppc`object_unref [inlined] 
object_finalize(data=0x42023e00) at object.c:707:5
     frame #12: 0x003a990c 
qemu-system-ppc`object_unref(objptr=0x42023e00) at object.c:1216:9
     frame #13: 0x00355114 qemu-system-ppc`address_space_dispatch_free 
at physmem.c:1001:9
     frame #14: 0x003550fc qemu-system-ppc`address_space_dispatch_free 
at physmem.c:1010:9
     frame #15: 0x003550e0 
qemu-system-ppc`address_space_dispatch_free(d=0x000060000385d680) at 
physmem.c:2473:5
     frame #16: 0x00349438 
qemu-system-ppc`flatview_destroy(view=0x000060000385d640) at memory.c:295:9
     frame #17: 0x00524920 
qemu-system-ppc`call_rcu_thread(opaque=<unavailable>) at rcu.c:301:13
     frame #18: 0x0051c1f0 
qemu-system-ppc`qemu_thread_start(args=<unavailable>) at 
qemu-thread-posix.c:541:9
     frame #19: 0x8b223034 libsystem_pthread.dylib`_pthread_start + 136

However when running 'make check-unit', qobject_is_equal_list_test()
is successful, so I'm a bit confused...

>           static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
> +        DeviceState *gpad;
>   
> -        gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
> -        gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
> -        gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
> -        gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
> -        gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
> +        gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
> +        for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
> +            qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
> +        }
> +        qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
>   
> -        stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
> +        qdev_connect_gpio_out(gpad, 0,
> +                              qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
> +        qdev_connect_gpio_out(gpad, 1,
> +                              qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
> +        qdev_connect_gpio_out(gpad, 2,
> +                              qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
> +        qdev_connect_gpio_out(gpad, 3,
> +                              qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
> +        qdev_connect_gpio_out(gpad, 4,
> +                              qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
>       }

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Peter,
>
> Cc'ing Markus for QObject.
>
> On 30/10/23 12:48, Peter Maydell wrote:
>> Convert the hw/input/stellaris_input device to qdev.
>> The interface uses an array property for the board to specify the
>> keycodes to use, so the s->keycodes memory is now allocated by the
>> array-property machinery.
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> v1->v2: drop private/public comment lines
>> ---
>>   include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>>   hw/arm/stellaris.c                   | 26 +++++++---
>>   hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
>>   3 files changed, 89 insertions(+), 32 deletions(-)
>
>
>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>> index 96585dd7106..707b0dae375 100644
>> --- a/hw/arm/stellaris.c
>> +++ b/hw/arm/stellaris.c
>> @@ -31,6 +31,7 @@
>>   #include "hw/timer/stellaris-gptm.h"
>>   #include "hw/qdev-clock.h"
>>   #include "qom/object.h"
>> +#include "qapi/qmp/qlist.h"
>>     #define GPIO_A 0
>>   #define GPIO_B 1
>> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>>           sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>>       }
>>       if (board->peripherals & BP_GAMEPAD) {
>> -        qemu_irq gpad_irq[5];
>> +        QList *gpad_keycode_list = qlist_new();
>
> I'm trying to understand better qlist_new(), but unfortunately there
> is not much documentation. Looking at how the allocated list was
> released, I found use of g_autoptr in tests/unit/check-qobject.c,
> so I tried:
>
>            g_autoptr(QList) gpad_keycode_list = qlist_new();

QObject and its subtypes QDict, QList, QString, ... are reference
counted.  qFOO_new() ist to be paired with qFOO_unref() or
qobject_unref().

Your use of g_autoptr(QList) should work.

> But QEMU crashes elsewhere which seems unrelated:
>
> * thread #2, stop reason = signal SIGABRT
>   * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
>     frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
>     frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
>     frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
>     frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
>     frame #5: 0x8b05b094 libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
>     frame #6: 0x8b05a2a8 libsystem_malloc.dylib`nanov2_allocate_outlined + 404
>     frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
>     frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage + 76
>     frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
>     frame #10: 0x003a9920 qemu-system-ppc`object_unref [inlined] object_property_del_all(obj=0x42023e00) at object.c:635:34
>     frame #11: 0x003a9914 qemu-system-ppc`object_unref [inlined] object_finalize(data=0x42023e00) at object.c:707:5
>     frame #12: 0x003a990c qemu-system-ppc`object_unref(objptr=0x42023e00) at object.c:1216:9
>     frame #13: 0x00355114 qemu-system-ppc`address_space_dispatch_free at physmem.c:1001:9
>     frame #14: 0x003550fc qemu-system-ppc`address_space_dispatch_free at physmem.c:1010:9
>     frame #15: 0x003550e0 qemu-system-ppc`address_space_dispatch_free(d=0x000060000385d680) at physmem.c:2473:5
>     frame #16: 0x00349438 qemu-system-ppc`flatview_destroy(view=0x000060000385d640) at memory.c:295:9
>     frame #17: 0x00524920 qemu-system-ppc`call_rcu_thread(opaque=<unavailable>) at rcu.c:301:13
>     frame #18: 0x0051c1f0 qemu-system-ppc`qemu_thread_start(args=<unavailable>) at qemu-thread-posix.c:541:9
>     frame #19: 0x8b223034 libsystem_pthread.dylib`_pthread_start + 136

Can't see QList or QObject in this backtrace.  object_unref() is QOM,
not QObject.

> However when running 'make check-unit', qobject_is_equal_list_test()
> is successful, so I'm a bit confused...
>
>>           static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
>> +        DeviceState *gpad;
>>   -        gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
>> -        gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
>> -        gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
>> -        gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
>> -        gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
>> +        gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
>> +        for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
>> +            qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
>> +        }
>> +        qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
>> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
>>   -        stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
>> +        qdev_connect_gpio_out(gpad, 0,
>> +                              qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
>> +        qdev_connect_gpio_out(gpad, 1,
>> +                              qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
>> +        qdev_connect_gpio_out(gpad, 2,
>> +                              qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
>> +        qdev_connect_gpio_out(gpad, 3,
>> +                              qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
>> +        qdev_connect_gpio_out(gpad, 4,
>> +                              qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
>>       }

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Peter Maydell]

On Mon, 30 Oct 2023 at 13:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> Cc'ing Markus for QObject.
>
> On 30/10/23 12:48, Peter Maydell wrote:
> > Convert the hw/input/stellaris_input device to qdev.
> >
> > The interface uses an array property for the board to specify the
> > keycodes to use, so the s->keycodes memory is now allocated by the
> > array-property machinery.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > v1->v2: drop private/public comment lines
> > ---
> >   include/hw/input/stellaris_gamepad.h | 22 ++++++++-
> >   hw/arm/stellaris.c                   | 26 +++++++---
> >   hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
> >   3 files changed, 89 insertions(+), 32 deletions(-)
>
>
> > diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> > index 96585dd7106..707b0dae375 100644
> > --- a/hw/arm/stellaris.c
> > +++ b/hw/arm/stellaris.c
> > @@ -31,6 +31,7 @@
> >   #include "hw/timer/stellaris-gptm.h"
> >   #include "hw/qdev-clock.h"
> >   #include "qom/object.h"
> > +#include "qapi/qmp/qlist.h"
> >
> >   #define GPIO_A 0
> >   #define GPIO_B 1
> > @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
> >           sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
> >       }
> >       if (board->peripherals & BP_GAMEPAD) {
> > -        qemu_irq gpad_irq[5];
> > +        QList *gpad_keycode_list = qlist_new();
>
> I'm trying to understand better qlist_new(), but unfortunately there
> is not much documentation. Looking at how the allocated list was
> released, I found use of g_autoptr in tests/unit/check-qobject.c,
> so I tried:
>
>             g_autoptr(QList) gpad_keycode_list = qlist_new();

The API for qdev_prop_set_array() documents that it takes ownership
of the list you pass it (and it ends up calling qobject_unref() on it).
So I think adding g_autoptr() here will result in the memory being
double-freed (once inside qobject_unref() when the refcount
goes to 0, and once when g_autoptr frees it as it goes out of scope)...

> * thread #2, stop reason = signal SIGABRT
>    * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
>      frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
>      frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
>      frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
>      frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
>      frame #5: 0x8b05b094
> libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
>      frame #6: 0x8b05a2a8
> libsystem_malloc.dylib`nanov2_allocate_outlined + 404
>      frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
>      frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage
> + 76
>      frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96

...which is probably why a later memory operation runs into a
malloc data corruption assertion.

thanks
-- PMM

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Philippe Mathieu-Daudé]

On 30/10/23 15:41, Peter Maydell wrote:
> On Mon, 30 Oct 2023 at 13:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> Cc'ing Markus for QObject.
>>
>> On 30/10/23 12:48, Peter Maydell wrote:
>>> Convert the hw/input/stellaris_input device to qdev.
>>>
>>> The interface uses an array property for the board to specify the
>>> keycodes to use, so the s->keycodes memory is now allocated by the
>>> array-property machinery.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> v1->v2: drop private/public comment lines
>>> ---
>>>    include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>>>    hw/arm/stellaris.c                   | 26 +++++++---
>>>    hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
>>>    3 files changed, 89 insertions(+), 32 deletions(-)
>>
>>
>>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>>> index 96585dd7106..707b0dae375 100644
>>> --- a/hw/arm/stellaris.c
>>> +++ b/hw/arm/stellaris.c
>>> @@ -31,6 +31,7 @@
>>>    #include "hw/timer/stellaris-gptm.h"
>>>    #include "hw/qdev-clock.h"
>>>    #include "qom/object.h"
>>> +#include "qapi/qmp/qlist.h"
>>>
>>>    #define GPIO_A 0
>>>    #define GPIO_B 1
>>> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>>>            sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>>>        }
>>>        if (board->peripherals & BP_GAMEPAD) {
>>> -        qemu_irq gpad_irq[5];
>>> +        QList *gpad_keycode_list = qlist_new();
>>
>> I'm trying to understand better qlist_new(), but unfortunately there
>> is not much documentation. Looking at how the allocated list was
>> released, I found use of g_autoptr in tests/unit/check-qobject.c,
>> so I tried:
>>
>>              g_autoptr(QList) gpad_keycode_list = qlist_new();
> 
> The API for qdev_prop_set_array() documents that it takes ownership
> of the list you pass it (and it ends up calling qobject_unref() on it).
> So I think adding g_autoptr() here will result in the memory being
> double-freed (once inside qobject_unref() when the refcount
> goes to 0, and once when g_autoptr frees it as it goes out of scope)...

Ah, I missed how qdev_prop_set_array() is involved.

>> * thread #2, stop reason = signal SIGABRT
>>     * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
>>       frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
>>       frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
>>       frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
>>       frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
>>       frame #5: 0x8b05b094
>> libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
>>       frame #6: 0x8b05a2a8
>> libsystem_malloc.dylib`nanov2_allocate_outlined + 404
>>       frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
>>       frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage
>> + 76
>>       frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
> 
> ...which is probably why a later memory operation runs into a
> malloc data corruption assertion.

Yes, this is certainly the reason. Thanks for the explanation!

Phil.

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Mark Cave-Ayland]

On 30/10/2023 11:48, Peter Maydell wrote:

> Convert the hw/input/stellaris_input device to qdev.
> 
> The interface uses an array property for the board to specify the
> keycodes to use, so the s->keycodes memory is now allocated by the
> array-property machinery.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> v1->v2: drop private/public comment lines
> ---
>   include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>   hw/arm/stellaris.c                   | 26 +++++++---
>   hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
>   3 files changed, 89 insertions(+), 32 deletions(-)
> 
> diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
> index 23cfd3c95f3..6140b889a28 100644
> --- a/include/hw/input/stellaris_gamepad.h
> +++ b/include/hw/input/stellaris_gamepad.h
> @@ -11,8 +11,26 @@
>   #ifndef HW_INPUT_STELLARIS_GAMEPAD_H
>   #define HW_INPUT_STELLARIS_GAMEPAD_H
>   
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
>   
> -/* stellaris_gamepad.c */
> -void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
> +/*
> + * QEMU interface:
> + *  + QOM array property "keycodes": uint32_t QEMU keycodes to handle
> + *  + unnamed GPIO outputs: one per keycode, in the same order as the
> + *    "keycodes" array property entries; asserted when key is down
> + */
> +
> +#define TYPE_STELLARIS_GAMEPAD "stellaris-gamepad"
> +OBJECT_DECLARE_SIMPLE_TYPE(StellarisGamepad, STELLARIS_GAMEPAD)
> +
> +struct StellarisGamepad {
> +    SysBusDevice parent_obj;

Minor style comment: for QOM types there should be an empty line after parent_obj to 
aid identification (as per 
https://qemu.readthedocs.io/en/master/devel/style.html#qemu-object-model-declarations).

> +    uint32_t num_buttons;
> +    qemu_irq *irqs;
> +    uint32_t *keycodes;
> +    uint8_t *pressed;
> +    int extension;
> +};
>   
>   #endif
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 96585dd7106..707b0dae375 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -31,6 +31,7 @@
>   #include "hw/timer/stellaris-gptm.h"
>   #include "hw/qdev-clock.h"
>   #include "qom/object.h"
> +#include "qapi/qmp/qlist.h"
>   
>   #define GPIO_A 0
>   #define GPIO_B 1
> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>           sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>       }
>       if (board->peripherals & BP_GAMEPAD) {
> -        qemu_irq gpad_irq[5];
> +        QList *gpad_keycode_list = qlist_new();
>           static const int gpad_keycode[5] = { 0xc8, 0xd0, 0xcb, 0xcd, 0x1d };
> +        DeviceState *gpad;
>   
> -        gpad_irq[0] = qemu_irq_invert(gpio_in[GPIO_E][0]); /* up */
> -        gpad_irq[1] = qemu_irq_invert(gpio_in[GPIO_E][1]); /* down */
> -        gpad_irq[2] = qemu_irq_invert(gpio_in[GPIO_E][2]); /* left */
> -        gpad_irq[3] = qemu_irq_invert(gpio_in[GPIO_E][3]); /* right */
> -        gpad_irq[4] = qemu_irq_invert(gpio_in[GPIO_F][1]); /* select */
> +        gpad = qdev_new(TYPE_STELLARIS_GAMEPAD);
> +        for (i = 0; i < ARRAY_SIZE(gpad_keycode); i++) {
> +            qlist_append_int(gpad_keycode_list, gpad_keycode[i]);
> +        }
> +        qdev_prop_set_array(gpad, "keycodes", gpad_keycode_list);
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(gpad), &error_fatal);
>   
> -        stellaris_gamepad_init(5, gpad_irq, gpad_keycode);
> +        qdev_connect_gpio_out(gpad, 0,
> +                              qemu_irq_invert(gpio_in[GPIO_E][0])); /* up */
> +        qdev_connect_gpio_out(gpad, 1,
> +                              qemu_irq_invert(gpio_in[GPIO_E][1])); /* down */
> +        qdev_connect_gpio_out(gpad, 2,
> +                              qemu_irq_invert(gpio_in[GPIO_E][2])); /* left */
> +        qdev_connect_gpio_out(gpad, 3,
> +                              qemu_irq_invert(gpio_in[GPIO_E][3])); /* right */
> +        qdev_connect_gpio_out(gpad, 4,
> +                              qemu_irq_invert(gpio_in[GPIO_F][1])); /* select */
>       }
>       for (i = 0; i < 7; i++) {
>           if (board->dc4 & (1 << i)) {
> diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
> index 82ddc47a26d..6ccf0e80adc 100644
> --- a/hw/input/stellaris_gamepad.c
> +++ b/hw/input/stellaris_gamepad.c
> @@ -8,19 +8,13 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qapi/error.h"
>   #include "hw/input/stellaris_gamepad.h"
>   #include "hw/irq.h"
> +#include "hw/qdev-properties.h"
>   #include "migration/vmstate.h"
>   #include "ui/console.h"
>   
> -typedef struct {
> -    uint32_t num_buttons;
> -    int extension;
> -    qemu_irq *irqs;
> -    uint32_t *keycodes;
> -    uint8_t *pressed;
> -} StellarisGamepad;
> -
>   static void stellaris_gamepad_put_key(void * opaque, int keycode)
>   {
>       StellarisGamepad *s = (StellarisGamepad *)opaque;
> @@ -57,22 +51,55 @@ static const VMStateDescription vmstate_stellaris_gamepad = {
>       }
>   };
>   
> -/* Returns an array of 5 output slots.  */
> -void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
> +static void stellaris_gamepad_realize(DeviceState *dev, Error **errp)
>   {
> -    StellarisGamepad *s;
> -    int i;
> +    StellarisGamepad *s = STELLARIS_GAMEPAD(dev);
>   
> -    s = g_new0(StellarisGamepad, 1);
> -    s->irqs = g_new0(qemu_irq, n);
> -    s->keycodes = g_new0(uint32_t, n);
> -    s->pressed = g_new0(uint8_t, n);
> -    for (i = 0; i < n; i++) {
> -        s->irqs[i] = irq[i];
> -        s->keycodes[i] = keycode[i];
> +    if (s->num_buttons == 0) {
> +        error_setg(errp, "keycodes property array must be set");
> +        return;
>       }
> -    s->num_buttons = n;
> -    qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
> -                     &vmstate_stellaris_gamepad, s);
> +
> +    s->irqs = g_new0(qemu_irq, s->num_buttons);
> +    s->pressed = g_new0(uint8_t, s->num_buttons);
> +    qdev_init_gpio_out(dev, s->irqs, s->num_buttons);
> +    qemu_add_kbd_event_handler(stellaris_gamepad_put_key, dev);
>   }
> +
> +static void stellaris_gamepad_reset_enter(Object *obj, ResetType type)
> +{
> +    StellarisGamepad *s = STELLARIS_GAMEPAD(obj);
> +
> +    memset(s->pressed, 0, s->num_buttons * sizeof(uint8_t));
> +}
> +
> +static Property stellaris_gamepad_properties[] = {
> +    DEFINE_PROP_ARRAY("keycodes", StellarisGamepad, num_buttons,
> +                      keycodes, qdev_prop_uint32, uint32_t),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void stellaris_gamepad_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +
> +    rc->phases.enter = stellaris_gamepad_reset_enter;
> +    dc->realize = stellaris_gamepad_realize;
> +    dc->vmsd = &vmstate_stellaris_gamepad;
> +    device_class_set_props(dc, stellaris_gamepad_properties);
> +}
> +
> +static const TypeInfo stellaris_gamepad_info = {
> +    .name = TYPE_STELLARIS_GAMEPAD,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(StellarisGamepad),
> +    .class_init = stellaris_gamepad_class_init,
> +};
> +
> +static void stellaris_gamepad_register_types(void)
> +{
> +    type_register_static(&stellaris_gamepad_info);
> +}
> +
> +type_init(stellaris_gamepad_register_types);

Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil 
has considered some automation to remove the type_init() boilerplate for the majority 
of cases.


ATB,

Mark.

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Philippe Mathieu-Daudé]

On 30/10/23 15:25, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Hi Peter,
>>
>> Cc'ing Markus for QObject.
>>
>> On 30/10/23 12:48, Peter Maydell wrote:
>>> Convert the hw/input/stellaris_input device to qdev.
>>> The interface uses an array property for the board to specify the
>>> keycodes to use, so the s->keycodes memory is now allocated by the
>>> array-property machinery.
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> v1->v2: drop private/public comment lines
>>> ---
>>>    include/hw/input/stellaris_gamepad.h | 22 ++++++++-
>>>    hw/arm/stellaris.c                   | 26 +++++++---
>>>    hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
>>>    3 files changed, 89 insertions(+), 32 deletions(-)
>>
>>
>>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>>> index 96585dd7106..707b0dae375 100644
>>> --- a/hw/arm/stellaris.c
>>> +++ b/hw/arm/stellaris.c
>>> @@ -31,6 +31,7 @@
>>>    #include "hw/timer/stellaris-gptm.h"
>>>    #include "hw/qdev-clock.h"
>>>    #include "qom/object.h"
>>> +#include "qapi/qmp/qlist.h"
>>>      #define GPIO_A 0
>>>    #define GPIO_B 1
>>> @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>>>            sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>>>        }
>>>        if (board->peripherals & BP_GAMEPAD) {
>>> -        qemu_irq gpad_irq[5];
>>> +        QList *gpad_keycode_list = qlist_new();
>>
>> I'm trying to understand better qlist_new(), but unfortunately there
>> is not much documentation. Looking at how the allocated list was
>> released, I found use of g_autoptr in tests/unit/check-qobject.c,
>> so I tried:
>>
>>             g_autoptr(QList) gpad_keycode_list = qlist_new();
> 
> QObject and its subtypes QDict, QList, QString, ... are reference
> counted.  qFOO_new() ist to be paired with qFOO_unref() or
> qobject_unref().
> 
> Your use of g_autoptr(QList) should work.

Peter figured qdev_prop_set_array() takes the ownership, so using
g_autoptr triggers a double-free:

https://lore.kernel.org/qemu-devel/CAFEAcA_GC8ypM2Y94KCU9Q_dntF6Na+igu-+0JZJ+MvPFE_HcA@mail.gmail.com/

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Peter Maydell]

On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 30/10/2023 11:48, Peter Maydell wrote:
>
> > Convert the hw/input/stellaris_input device to qdev.
> >
> > The interface uses an array property for the board to specify the
> > keycodes to use, so the s->keycodes memory is now allocated by the
> > array-property machinery.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > v1->v2: drop private/public comment lines
> > ---
> >   include/hw/input/stellaris_gamepad.h | 22 ++++++++-
> >   hw/arm/stellaris.c                   | 26 +++++++---
> >   hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
> >   3 files changed, 89 insertions(+), 32 deletions(-)
> >
> > diff --git a/include/hw/input/stellaris_gamepad.h b/include/hw/input/stellaris_gamepad.h
> > index 23cfd3c95f3..6140b889a28 100644
> > --- a/include/hw/input/stellaris_gamepad.h
> > +++ b/include/hw/input/stellaris_gamepad.h
> > @@ -11,8 +11,26 @@
> >   #ifndef HW_INPUT_STELLARIS_GAMEPAD_H
> >   #define HW_INPUT_STELLARIS_GAMEPAD_H
> >
> > +#include "hw/sysbus.h"
> > +#include "qom/object.h"
> >
> > -/* stellaris_gamepad.c */
> > -void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode);
> > +/*
> > + * QEMU interface:
> > + *  + QOM array property "keycodes": uint32_t QEMU keycodes to handle
> > + *  + unnamed GPIO outputs: one per keycode, in the same order as the
> > + *    "keycodes" array property entries; asserted when key is down
> > + */
> > +
> > +#define TYPE_STELLARIS_GAMEPAD "stellaris-gamepad"
> > +OBJECT_DECLARE_SIMPLE_TYPE(StellarisGamepad, STELLARIS_GAMEPAD)
> > +
> > +struct StellarisGamepad {
> > +    SysBusDevice parent_obj;
>
> Minor style comment: for QOM types there should be an empty line after parent_obj to
> aid identification (as per
> https://qemu.readthedocs.io/en/master/devel/style.html#qemu-object-model-declarations).

Fixed.

> > +static const TypeInfo stellaris_gamepad_info = {
> > +    .name = TYPE_STELLARIS_GAMEPAD,
> > +    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(StellarisGamepad),
> > +    .class_init = stellaris_gamepad_class_init,
> > +};
> > +
> > +static void stellaris_gamepad_register_types(void)
> > +{
> > +    type_register_static(&stellaris_gamepad_info);
> > +}
> > +
> > +type_init(stellaris_gamepad_register_types);
>
> Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
> has considered some automation to remove the type_init() boilerplate for the majority
> of cases.

I could, I guess. It seems a bit awkward that DEFINE_TYPES()
wants you to pass it an array even when you only have one type,
though, which is going to be a very common use case.

-- PMM

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Peter Maydell]

On Tue, 31 Oct 2023 at 13:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> >
> > On 30/10/2023 11:48, Peter Maydell wrote:
> > Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
> > has considered some automation to remove the type_init() boilerplate for the majority
> > of cases.
>
> I could, I guess. It seems a bit awkward that DEFINE_TYPES()
> wants you to pass it an array even when you only have one type,
> though, which is going to be a very common use case.

I'm going to squash this into this patch:
diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
index 6ccf0e80adc..d42ba4f0582 100644
--- a/hw/input/stellaris_gamepad.c
+++ b/hw/input/stellaris_gamepad.c
@@ -90,16 +90,13 @@ static void
stellaris_gamepad_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, stellaris_gamepad_properties);
 }

-static const TypeInfo stellaris_gamepad_info = {
-    .name = TYPE_STELLARIS_GAMEPAD,
-    .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(StellarisGamepad),
-    .class_init = stellaris_gamepad_class_init,
+static const TypeInfo stellaris_gamepad_info[] = {
+    {
+        .name = TYPE_STELLARIS_GAMEPAD,
+        .parent = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(StellarisGamepad),
+        .class_init = stellaris_gamepad_class_init,
+    },
 };

-static void stellaris_gamepad_register_types(void)
-{
-    type_register_static(&stellaris_gamepad_info);
-}
-
-type_init(stellaris_gamepad_register_types);
+DEFINE_TYPES(stellaris_gamepad_info);


The array is a bit awkward, but it's overall better than having
to define the register-types function.

thanks
-- PMM

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Philippe Mathieu-Daudé]

On 31/10/23 15:05, Peter Maydell wrote:
> On Tue, 31 Oct 2023 at 13:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> On 30/10/2023 11:48, Peter Maydell wrote:
>>> Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
>>> has considered some automation to remove the type_init() boilerplate for the majority
>>> of cases.
>>
>> I could, I guess. It seems a bit awkward that DEFINE_TYPES()
>> wants you to pass it an array even when you only have one type,
>> though, which is going to be a very common use case.

For single type, there is no point beside enforcing a QOM style.

I'll update docs/devel/qom.rst...

> I'm going to squash this into this patch:
> diff --git a/hw/input/stellaris_gamepad.c b/hw/input/stellaris_gamepad.c
> index 6ccf0e80adc..d42ba4f0582 100644
> --- a/hw/input/stellaris_gamepad.c
> +++ b/hw/input/stellaris_gamepad.c
> @@ -90,16 +90,13 @@ static void
> stellaris_gamepad_class_init(ObjectClass *klass, void *data)
>       device_class_set_props(dc, stellaris_gamepad_properties);
>   }
> 
> -static const TypeInfo stellaris_gamepad_info = {
> -    .name = TYPE_STELLARIS_GAMEPAD,
> -    .parent = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(StellarisGamepad),
> -    .class_init = stellaris_gamepad_class_init,
> +static const TypeInfo stellaris_gamepad_info[] = {
> +    {
> +        .name = TYPE_STELLARIS_GAMEPAD,
> +        .parent = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(StellarisGamepad),
> +        .class_init = stellaris_gamepad_class_init,
> +    },
>   };
> 
> -static void stellaris_gamepad_register_types(void)
> -{
> -    type_register_static(&stellaris_gamepad_info);
> -}
> -
> -type_init(stellaris_gamepad_register_types);
> +DEFINE_TYPES(stellaris_gamepad_info);
> 
> 
> The array is a bit awkward, but it's overall better than having
> to define the register-types function.

Thank you!

Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Peter Maydell]

On Tue, 31 Oct 2023 at 14:55, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 31/10/23 15:05, Peter Maydell wrote:
> > On Tue, 31 Oct 2023 at 13:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Mon, 30 Oct 2023 at 20:38, Mark Cave-Ayland
> >> <mark.cave-ayland@ilande.co.uk> wrote:
> >>>
> >>> On 30/10/2023 11:48, Peter Maydell wrote:
> >>> Is it worth converting this to use DEFINE_TYPES() during the conversion? I know Phil
> >>> has considered some automation to remove the type_init() boilerplate for the majority
> >>> of cases.
> >>
> >> I could, I guess. It seems a bit awkward that DEFINE_TYPES()
> >> wants you to pass it an array even when you only have one type,
> >> though, which is going to be a very common use case.
>
> For single type, there is no point beside enforcing a QOM style.
>
> I'll update docs/devel/qom.rst...

I do like that the macro means you're not writing an actual
function for the registration.

We could I guess have a DEFINE_TYPE() macro that was similar
to DEFINE_TYPES but emitted a function that called
type_register_static() instead of type_register_static_array().
Is that worth having? I'm not sure.

thanks
-- PMM