[PATCH 0/2] hw/char/sh_serial: QOM housekeeping

[PATCH 0/2] hw/char/sh_serial: QOM housekeeping

[Philippe Mathieu-Daudé]

- Parity in realize / unrealize
- Define TypeInfo structure

Philippe Mathieu-Daudé (2):
  hw/char/sh_serial: Delete fifo_timeout_timer in DeviceUnrealize
  hw/char/sh_serial: Convert to TypeInfo

 hw/char/sh_serial.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

-- 
2.47.1

[PATCH 1/2] hw/char/sh_serial: Delete fifo_timeout_timer in DeviceUnrealize

[Philippe Mathieu-Daudé]

fifo_timeout_timer is created in the DeviceRealize handler,
not in the instance_init one. For parity, delete it in
DeviceUnrealize, rather than instance_finalize.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/sh_serial.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 247aeb071ac..29ac9f9e5e7 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -436,9 +436,9 @@ static void sh_serial_realize(DeviceState *d, Error **errp)
     s->etu = NANOSECONDS_PER_SECOND / 9600;
 }
 
-static void sh_serial_finalize(Object *obj)
+static void sh_serial_unrealize(DeviceState *dev)
 {
-    SHSerialState *s = SH_SERIAL(obj);
+    SHSerialState *s = SH_SERIAL(dev);
 
     timer_del(&s->fifo_timeout_timer);
 }
@@ -447,6 +447,10 @@ static void sh_serial_init(Object *obj)
 {
 }
 
+static void sh_serial_finalize(Object *obj)
+{
+}
+
 static const Property sh_serial_properties[] = {
     DEFINE_PROP_CHR("chardev", SHSerialState, chr),
     DEFINE_PROP_UINT8("features", SHSerialState, feat, 0),
@@ -458,6 +462,7 @@ static void sh_serial_class_init(ObjectClass *oc, void *data)
 
     device_class_set_props(dc, sh_serial_properties);
     dc->realize = sh_serial_realize;
+    dc->unrealize = sh_serial_unrealize;
     device_class_set_legacy_reset(dc, sh_serial_reset);
     /* Reason: part of SuperH CPU/SoC, needs to be wired up */
     dc->user_creatable = false;
-- 
2.47.1

Re: [PATCH 1/2] hw/char/sh_serial: Delete fifo_timeout_timer in DeviceUnrealize

[Peter Maydell]

On Fri, 24 Jan 2025 at 17:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> fifo_timeout_timer is created in the DeviceRealize handler,
> not in the instance_init one. For parity, delete it in
> DeviceUnrealize, rather than instance_finalize.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/2] hw/char/sh_serial: Convert to TypeInfo

[Philippe Mathieu-Daudé]

QOM types are now registered using as TypeInfo via DEFINE_TYPES()
or type_init(). Update TYPE_SH_SERIAL, removing the empty QOM
instance_init/finalize handlers.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/sh_serial.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 29ac9f9e5e7..b1db91656fe 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -78,10 +78,6 @@ struct SHSerialState {
     qemu_irq bri;
 };
 
-typedef struct {} SHSerialStateClass;
-
-OBJECT_DEFINE_TYPE(SHSerialState, sh_serial, SH_SERIAL, SYS_BUS_DEVICE)
-
 static void sh_serial_clear_fifo(SHSerialState *s)
 {
     memset(s->rx_fifo, 0, SH_RX_FIFO_LENGTH);
@@ -443,14 +439,6 @@ static void sh_serial_unrealize(DeviceState *dev)
     timer_del(&s->fifo_timeout_timer);
 }
 
-static void sh_serial_init(Object *obj)
-{
-}
-
-static void sh_serial_finalize(Object *obj)
-{
-}
-
 static const Property sh_serial_properties[] = {
     DEFINE_PROP_CHR("chardev", SHSerialState, chr),
     DEFINE_PROP_UINT8("features", SHSerialState, feat, 0),
@@ -467,3 +455,14 @@ static void sh_serial_class_init(ObjectClass *oc, void *data)
     /* Reason: part of SuperH CPU/SoC, needs to be wired up */
     dc->user_creatable = false;
 }
+
+static const TypeInfo sh_serial_types[] = {
+    {
+        .name           = TYPE_SH_SERIAL,
+        .parent         = TYPE_SYS_BUS_DEVICE,
+        .instance_size  = sizeof(SHSerialState),
+        .class_init     = sh_serial_class_init,
+    },
+};
+
+DEFINE_TYPES(sh_serial_types)
-- 
2.47.1

Re: [PATCH 2/2] hw/char/sh_serial: Convert to TypeInfo

[Philippe Mathieu-Daudé]

On 24/1/25 18:50, Philippe Mathieu-Daudé wrote:
> QOM types are now registered using as TypeInfo via DEFINE_TYPES()
> or type_init(). Update TYPE_SH_SERIAL, removing the empty QOM
> instance_init/finalize handlers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/char/sh_serial.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
> index 29ac9f9e5e7..b1db91656fe 100644
> --- a/hw/char/sh_serial.c
> +++ b/hw/char/sh_serial.c
> @@ -78,10 +78,6 @@ struct SHSerialState {
>       qemu_irq bri;
>   };
>   
> -typedef struct {} SHSerialStateClass;

Note this structure was buggy, as it should have embedded its parent...

   struct SHSerialStateClass {
     SysBusDeviceClass parent_class;
     ...
   };

> -
> -OBJECT_DEFINE_TYPE(SHSerialState, sh_serial, SH_SERIAL, SYS_BUS_DEVICE)
> -
>   static void sh_serial_clear_fifo(SHSerialState *s)
>   {
>       memset(s->rx_fifo, 0, SH_RX_FIFO_LENGTH);
> @@ -443,14 +439,6 @@ static void sh_serial_unrealize(DeviceState *dev)
>       timer_del(&s->fifo_timeout_timer);
>   }
>   
> -static void sh_serial_init(Object *obj)
> -{
> -}
> -
> -static void sh_serial_finalize(Object *obj)
> -{
> -}
> -
>   static const Property sh_serial_properties[] = {
>       DEFINE_PROP_CHR("chardev", SHSerialState, chr),
>       DEFINE_PROP_UINT8("features", SHSerialState, feat, 0),
> @@ -467,3 +455,14 @@ static void sh_serial_class_init(ObjectClass *oc, void *data)
>       /* Reason: part of SuperH CPU/SoC, needs to be wired up */
>       dc->user_creatable = false;
>   }
> +
> +static const TypeInfo sh_serial_types[] = {
> +    {
> +        .name           = TYPE_SH_SERIAL,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(SHSerialState),
> +        .class_init     = sh_serial_class_init,
> +    },
> +};
> +
> +DEFINE_TYPES(sh_serial_types)

Re: [PATCH 2/2] hw/char/sh_serial: Convert to TypeInfo

[Peter Maydell]

On Fri, 24 Jan 2025 at 17:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> QOM types are now registered using as TypeInfo via DEFINE_TYPES()
> or type_init(). Update TYPE_SH_SERIAL, removing the empty QOM
> instance_init/finalize handlers.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/char/sh_serial.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
> index 29ac9f9e5e7..b1db91656fe 100644
> --- a/hw/char/sh_serial.c
> +++ b/hw/char/sh_serial.c
> @@ -78,10 +78,6 @@ struct SHSerialState {
>      qemu_irq bri;
>  };
>
> -typedef struct {} SHSerialStateClass;
> -
> -OBJECT_DEFINE_TYPE(SHSerialState, sh_serial, SH_SERIAL, SYS_BUS_DEVICE)
> -

This was definitely wrong, because OBJECT_DEFINE_TYPE()
is only for cases where the class needs its own virtual
methods or some other per-class state in its own class struct.

>  static void sh_serial_clear_fifo(SHSerialState *s)
>  {
>      memset(s->rx_fifo, 0, SH_RX_FIFO_LENGTH);
> @@ -443,14 +439,6 @@ static void sh_serial_unrealize(DeviceState *dev)
>      timer_del(&s->fifo_timeout_timer);
>  }
>
> -static void sh_serial_init(Object *obj)
> -{
> -}
> -
> -static void sh_serial_finalize(Object *obj)
> -{
> -}
> -
>  static const Property sh_serial_properties[] = {
>      DEFINE_PROP_CHR("chardev", SHSerialState, chr),
>      DEFINE_PROP_UINT8("features", SHSerialState, feat, 0),
> @@ -467,3 +455,14 @@ static void sh_serial_class_init(ObjectClass *oc, void *data)
>      /* Reason: part of SuperH CPU/SoC, needs to be wired up */
>      dc->user_creatable = false;
>  }
> +
> +static const TypeInfo sh_serial_types[] = {
> +    {
> +        .name           = TYPE_SH_SERIAL,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(SHSerialState),
> +        .class_init     = sh_serial_class_init,
> +    },
> +};
> +
> +DEFINE_TYPES(sh_serial_types)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Do you have a view on when we should:
 * use DEFINE_TYPES like this
 * longhand write out a type_init() and a function
   (maybe only if you need to programmatically construct
   the type structs, e.g. in a loop ?)
 * use OBJECT_DEFINE_TYPE()
?

Currently docs/devel/qom.rst leads off with the
"write it out longhand" approach, then mentions
DEFINE_TYPES for if you want to register "several
static types", and finally documents the OBJECT_DECLARE
and OBJECT_DEFINE families of macros.

thanks
-- PMM

Re: [PATCH 0/2] hw/char/sh_serial: QOM housekeeping

[Philippe Mathieu-Daudé]

On 24/1/25 18:50, Philippe Mathieu-Daudé wrote:
> - Parity in realize / unrealize
> - Define TypeInfo structure
> 
> Philippe Mathieu-Daudé (2):
>    hw/char/sh_serial: Delete fifo_timeout_timer in DeviceUnrealize
>    hw/char/sh_serial: Convert to TypeInfo
> 
>   hw/char/sh_serial.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
> 

ping?