On 01/31/2012 04:00 PM, Jan Kiszka wrote:
On 2012-01-31 21:49, Anthony Liguori wrote: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@xxxxxxxxxxx>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.Yes, but this is a callback for the audio subsystem, look at arch_init.c. Nothing we can do here, only if we change that callback prototype.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.Yep, there is room for improvements. The audio system is just another backend, like a chardev or netdev. It should once we handled like this I guess.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.Oops, will fix.+ + 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?Once it's properly usable, I will do so. So far I see now in-tree - ideally type-checking - replacement for qdev_prop_set_ptr.
Why is what's in the tree not usable?Just don't do pcspk_init as a static inline (which is not that nice to do anyway) and you don't need to worry about the availability of an accessor.
And BTW, there is strict type checking now, which makes it already an improvement over property pointers.
Regards, Anthony Liguori
Jan
-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
