Re: [PATCH] hw/char/mcf_uart: Have mcf_uart_create() return DeviceState

[Thomas Huth]

Am Thu, 19 Oct 2023 12:49:29 +0200
schrieb Philippe Mathieu-Daudé <philmd@linaro.org>:

> There is no point in having mcf_uart_init() demote the DeviceState
> pointer and return a void one. Directly return the real typedef.
> 
> mcf_uart_init() do both init + realize: rename as mcf_uart_create().
> 
> Similarly, mcf_uart_mm_init() do init / realize / mmap: rename as
> mcf_uart_create_mmap().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/m68k/mcf.h |  4 ++--
>  hw/char/mcf_uart.c    | 13 +++++++------
>  hw/m68k/mcf5206.c     |  6 +++---
>  hw/m68k/mcf5208.c     |  6 +++---
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/m68k/mcf.h b/include/hw/m68k/mcf.h
> index 8cbd587bbf..5d9f876ffe 100644
> --- a/include/hw/m68k/mcf.h
> +++ b/include/hw/m68k/mcf.h
> @@ -10,8 +10,8 @@ uint64_t mcf_uart_read(void *opaque, hwaddr addr,
>                         unsigned size);
>  void mcf_uart_write(void *opaque, hwaddr addr,
>                      uint64_t val, unsigned size);
> -void *mcf_uart_init(qemu_irq irq, Chardev *chr);
> -void mcf_uart_mm_init(hwaddr base, qemu_irq irq, Chardev *chr);
> +DeviceState *mcf_uart_create(qemu_irq irq, Chardev *chr);
> +DeviceState *mcf_uart_create_mmap(hwaddr base, qemu_irq irq, Chardev *chr);
>  
>  /* mcf_intc.c */
>  qemu_irq *mcf_intc_init(struct MemoryRegion *sysmem,
> diff --git a/hw/char/mcf_uart.c b/hw/char/mcf_uart.c
> index 6fa4ac502c..f9cbc9bdc4 100644
> --- a/hw/char/mcf_uart.c
> +++ b/hw/char/mcf_uart.c
> @@ -342,25 +342,26 @@ static void mcf_uart_register(void)
>  
>  type_init(mcf_uart_register)
>  
> -void *mcf_uart_init(qemu_irq irq, Chardev *chrdrv)
> +DeviceState *mcf_uart_create(qemu_irq irq, Chardev *chrdrv)
>  {
> -    DeviceState  *dev;
> +    DeviceState *dev;
>  
>      dev = qdev_new(TYPE_MCF_UART);
>      if (chrdrv) {
>          qdev_prop_set_chr(dev, "chardev", chrdrv);
>      }
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -
>      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
>  
>      return dev;
>  }
>  
> -void mcf_uart_mm_init(hwaddr base, qemu_irq irq, Chardev *chrdrv)
> +DeviceState *mcf_uart_create_mmap(hwaddr base, qemu_irq irq, Chardev *chrdrv)
>  {
> -    DeviceState  *dev;
> +    DeviceState *dev;
>  
> -    dev = mcf_uart_init(irq, chrdrv);
> +    dev = mcf_uart_create(irq, chrdrv);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +
> +    return dev;
>  }

Changing the return type (and name) of mcf_uart_mm_init() seems to be
unnecessary. Could you please drop that change?

 Thomas


> diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
> index 2ab1b4f059..673861574c 100644
> --- a/hw/m68k/mcf5206.c
> +++ b/hw/m68k/mcf5206.c
> @@ -167,7 +167,7 @@ typedef struct {
>      MemoryRegion iomem;
>      qemu_irq *pic;
>      m5206_timer_state *timer[2];
> -    void *uart[2];
> +    DeviceState *uart[2];
>      uint8_t scr;
>      uint8_t icr[14];
>      uint16_t imr; /* 1 == interrupt is masked.  */
> @@ -599,8 +599,8 @@ static void mcf5206_mbar_realize(DeviceState *dev, Error 
> **errp)
>      s->pic = qemu_allocate_irqs(m5206_mbar_set_irq, s, 14);
>      s->timer[0] = m5206_timer_init(s->pic[9]);
>      s->timer[1] = m5206_timer_init(s->pic[10]);
> -    s->uart[0] = mcf_uart_init(s->pic[12], serial_hd(0));
> -    s->uart[1] = mcf_uart_init(s->pic[13], serial_hd(1));
> +    s->uart[0] = mcf_uart_create(s->pic[12], serial_hd(0));
> +    s->uart[1] = mcf_uart_create(s->pic[13], serial_hd(1));
>      s->cpu = M68K_CPU(qemu_get_cpu(0));
>  }
>  
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index be1033f84f..d22d8536db 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -261,9 +261,9 @@ static void mcf5208evb_init(MachineState *machine)
>      /* Internal peripherals.  */
>      pic = mcf_intc_init(address_space_mem, 0xfc048000, cpu);
>  
> -    mcf_uart_mm_init(0xfc060000, pic[26], serial_hd(0));
> -    mcf_uart_mm_init(0xfc064000, pic[27], serial_hd(1));
> -    mcf_uart_mm_init(0xfc068000, pic[28], serial_hd(2));
> +    mcf_uart_create_mmap(0xfc060000, pic[26], serial_hd(0));
> +    mcf_uart_create_mmap(0xfc064000, pic[27], serial_hd(1));
> +    mcf_uart_create_mmap(0xfc068000, pic[28], serial_hd(2));