Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/31/2012 11:41 AM, Jan Kiszka wrote:
> Convert the PC speaker device to a qdev ISA model. Move the public
> interface to a dedicated header file at this chance.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>

Heh, I did this too more or less the same way.  Some comments below:

> ---
>   arch_init.c    |    1 +
>   hw/i82378.c    |    3 +-
>   hw/mips_jazz.c |    3 +-
>   hw/pc.c        |    3 +-
>   hw/pc.h        |    4 ---
>   hw/pcspk.c     |   62 ++++++++++++++++++++++++++++++++++++++++++++++---------
>   hw/pcspk.h     |   45 ++++++++++++++++++++++++++++++++++++++++
>   7 files changed, 104 insertions(+), 17 deletions(-)
>   create mode 100644 hw/pcspk.h
>
> diff --git a/arch_init.c b/arch_init.c
> index 2366511..a45485b 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -42,6 +42,7 @@
>   #include "gdbstub.h"
>   #include "hw/smbios.h"
>   #include "exec-memory.h"
> +#include "hw/pcspk.h"
>
>   #ifdef TARGET_SPARC
>   int graphic_width = 1024;
> diff --git a/hw/i82378.c b/hw/i82378.c
> index 127cadf..79fa8b7 100644
> --- a/hw/i82378.c
> +++ b/hw/i82378.c
> @@ -20,6 +20,7 @@
>   #include "pci.h"
>   #include "pc.h"
>   #include "i8254.h"
> +#include "pcspk.h"
>
>   //#define DEBUG_I82378
>
> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, I82378State *s)
>       pit = pit_init(isabus, 0x40, 0, NULL);
>
>       /* speaker */
> -    pcspk_init(pit);
> +    pcspk_init(isabus, pit);
>
>       /* 2 82C37 (dma) */
>       DMA_init(1,&s->out[1]);
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index b61b218..65608dc 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -37,6 +37,7 @@
>   #include "loader.h"
>   #include "mc146818rtc.h"
>   #include "i8254.h"
> +#include "pcspk.h"
>   #include "blockdev.h"
>   #include "sysbus.h"
>   #include "exec-memory.h"
> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion *address_space,
>       cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
>       DMA_init(0, cpu_exit_irq);
>       pit = pit_init(isa_bus, 0x40, 0, NULL);
> -    pcspk_init(pit);
> +    pcspk_init(isa_bus, pit);
>
>       /* ISA IO space at 0x90000000 */
>       isa_mmio_init(0x90000000, 0x01000000);
> diff --git a/hw/pc.c b/hw/pc.c
> index 6fb1de7..f6a91a9 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -37,6 +37,7 @@
>   #include "multiboot.h"
>   #include "mc146818rtc.h"
>   #include "i8254.h"
> +#include "pcspk.h"
>   #include "msi.h"
>   #include "sysbus.h"
>   #include "sysemu.h"
> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>           /* connect PIT to output control line of the HPET */
>           qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0));
>       }
> -    pcspk_init(pit);
> +    pcspk_init(isa_bus, pit);
>
>       for(i = 0; i<  MAX_SERIAL_PORTS; i++) {
>           if (serial_hds[i]) {
> diff --git a/hw/pc.h b/hw/pc.h
> index b08708d..1b47bbd 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
>   /* hpet.c */
>   extern int no_hpet;
>
> -/* pcspk.c */
> -void pcspk_init(ISADevice *pit);
> -int pcspk_audio_init(ISABus *bus);
> -
>   /* piix_pci.c */
>   struct PCII440FXState;
>   typedef struct PCII440FXState PCII440FXState;
> diff --git a/hw/pcspk.c b/hw/pcspk.c
> index 43df818..5bd9e32 100644
> --- a/hw/pcspk.c
> +++ b/hw/pcspk.c
> @@ -28,6 +28,7 @@
>   #include "audio/audio.h"
>   #include "qemu-timer.h"
>   #include "i8254.h"
> +#include "pcspk.h"
>
>   #define PCSPK_BUF_LEN 1792
>   #define PCSPK_SAMPLE_RATE 32000
> @@ -35,10 +36,13 @@
>   #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / PCSPK_MAX_FREQ)
>
>   typedef struct {
> +    ISADevice dev;
> +    MemoryRegion ioport;
> +    uint32_t iobase;
>       uint8_t sample_buf[PCSPK_BUF_LEN];
>       QEMUSoundCard card;
>       SWVoiceOut *voice;
> -    ISADevice *pit;
> +    void *pit;
>       unsigned int pit_count;
>       unsigned int samples;
>       unsigned int play_pos;
> @@ -47,7 +51,7 @@ typedef struct {
>   } PCSpkState;
>
>   static const char *s_spk = "pcspk";
> -static PCSpkState pcspk_state;
> +static PCSpkState *pcspk_state;
>
>   static inline void generate_samples(PCSpkState *s)
>   {
> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free)
>
>   int pcspk_audio_init(ISABus *bus)
>   {
> -    PCSpkState *s =&pcspk_state;
> +    PCSpkState *s = pcspk_state;
>       struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0};


This can be a follow up, but what this is really doing is enabling audio for 
this device.  ISABus is unused as a parameter.

I think it would be better to make this a qdev bool property (audio_enabled) and 
then modify the code that calls this function to either set the property as a 
global, or use the QOM path to specifically set it on this device.

Either way, I think setting an audio_enabled flag via qdev makes a whole lot 
more sense conceptionally than using the -soundhw option.

>
>       AUD_register_card(s_spk,&s->card);
> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus)
>       return 0;
>   }
>
> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr,
> +                              unsigned size)
>   {
>       PCSpkState *s = opaque;
>       int out;
> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
>       return pit_get_gate(s->pit, 2) | (s->data_on<<  1) | s->dummy_refresh_clock | out;
>   }
>
> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr, uint64_t val,
> +                           unsigned size)
>   {
>       PCSpkState *s = opaque;
>       const int gate = val&  1;
> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>       }
>   }
>
> -void pcspk_init(ISADevice *pit)
> +static const MemoryRegionOps pcspk_io_ops = {
> +    .read = pcspk_io_read,
> +    .write = pcspk_io_write,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static int pcspk_initfn(ISADevice *dev)
> +{
> +    PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
> +
> +    memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1);
> +    isa_register_ioport(NULL,&s->ioport, s->iobase);

Should pass dev as the first argument to isa_register_ioport.  Otherwise the 
resource won't be cleaned up during destruction.

> +
> +    pcspk_state = s;
> +
> +    return 0;
> +}
> +
> +static void pcspk_class_initfn(ObjectClass *klass, void *data)
>   {
> -    PCSpkState *s =&pcspk_state;
> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> +    ic->init = pcspk_initfn;
> +}
>
> -    s->pit = pit;
> -    register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s);
> -    register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s);
> +static DeviceInfo pcspk_info = {
> +    .name       = "isa-pcspk",
> +    .size       = sizeof(PCSpkState),
> +    .no_user    = 1,
> +    .props      = (Property[]) {
> +        DEFINE_PROP_HEX32("iobase", PCSpkState, iobase,  -1),
> +        DEFINE_PROP_PTR("pit", PCSpkState, pit),

Please don't introduce a pointer property here.  They cannot be used in a 
meaningful way in qdev.  Why not register a link<TYPE_PIT> in instance_init?

Regards,

Anthony Liguori

> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +    .class_init = pcspk_class_initfn,
> +};
> +
> +static void pcspk_register(void)
> +{
> +    isa_qdev_register(&pcspk_info);
>   }
> +device_init(pcspk_register)
> diff --git a/hw/pcspk.h b/hw/pcspk.h
> new file mode 100644
> index 0000000..7f42bac
> --- /dev/null
> +++ b/hw/pcspk.h
> @@ -0,0 +1,45 @@
> +/*
> + * QEMU PC speaker emulation
> + *
> + * Copyright (c) 2006 Joachim Henke
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_PCSPK_H
> +#define HW_PCSPK_H
> +
> +#include "hw.h"
> +#include "isa.h"
> +
> +static inline ISADevice *pcspk_init(ISABus *bus, ISADevice *pit)
> +{
> +    ISADevice *dev;
> +
> +    dev = isa_create(bus, "isa-pcspk");
> +    qdev_prop_set_uint32(&dev->qdev, "iobase", 0x61);
> +    qdev_prop_set_ptr(&dev->qdev, "pit", pit);
> +    qdev_init_nofail(&dev->qdev);
> +
> +    return dev;
> +}
> +
> +int pcspk_audio_init(ISABus *bus);
> +
> +#endif /* !HW_PCSPK_H */