From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51540) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsKeM-0000kM-7H for qemu-devel@nongnu.org; Tue, 31 Jan 2012 15:50:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsKeH-0004HY-Eb for qemu-devel@nongnu.org; Tue, 31 Jan 2012 15:49:58 -0500 Received: from mail-pw0-f45.google.com ([209.85.160.45]:52300) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsKeH-0004HQ-4b for qemu-devel@nongnu.org; Tue, 31 Jan 2012 15:49:53 -0500 Received: by pbaa11 with SMTP id a11so628191pba.4 for ; Tue, 31 Jan 2012 12:49:51 -0800 (PST) Message-ID: <4F2853EA.3030808@codemonkey.ws> Date: Tue, 31 Jan 2012 14:49:46 -0600 From: Anthony Liguori MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Anthony Liguori , kvm , Marcelo Tosatti , qemu-devel , Blue Swirl , Avi Kivity 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 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 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 */