From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34072) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsLkO-00085u-Dk for qemu-devel@nongnu.org; Tue, 31 Jan 2012 17:00:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsLkI-0004eE-8z for qemu-devel@nongnu.org; Tue, 31 Jan 2012 17:00:16 -0500 Received: from fmmailgate01.web.de ([217.72.192.221]:47682) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsLkH-0004dR-OU for qemu-devel@nongnu.org; Tue, 31 Jan 2012 17:00:10 -0500 Received: from moweb002.kundenserver.de (moweb002.kundenserver.de [172.19.20.108]) by fmmailgate01.web.de (Postfix) with ESMTP id 27FF81A9C9E8B for ; Tue, 31 Jan 2012 23:00:08 +0100 (CET) Message-ID: <4F286463.4070807@web.de> Date: Tue, 31 Jan 2012 23:00:03 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <4F2853EA.3030808@codemonkey.ws> In-Reply-To: <4F2853EA.3030808@codemonkey.ws> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig3EFB9BA385D478AA68376E5F" 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: Anthony Liguori Cc: Anthony Liguori , kvm , Marcelo Tosatti , qemu-devel , Blue Swirl , Avi Kivity This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig3EFB9BA385D478AA68376E5F Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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 >=20 > Heh, I did this too more or less the same way. Some comments below: >=20 >> --- >> 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 =3D 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 =3D 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 =3D qemu_allocate_irqs(cpu_request_exit, NULL, 1); >> DMA_init(0, cpu_exit_irq); >> pit =3D 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 =3D 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 =3D "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 =3D&pcspk_state; >> + PCSpkState *s =3D pcspk_state; >> struct audsettings as =3D {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0};= >=20 >=20 > This can be a follow up, but what this is really doing is enabling audi= o > 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. >=20 > 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 specificall= y > set it on this device. >=20 > Either way, I think setting an audio_enabled flag via qdev makes a whol= e > 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. >=20 >> >> 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 =3D 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 =3D opaque; >> const int gate =3D 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 =3D { >> + .read =3D pcspk_io_read, >> + .write =3D pcspk_io_write, >> + .impl =3D { >> + .min_access_size =3D 1, >> + .max_access_size =3D 1, >> + }, >> +}; >> + >> +static int pcspk_initfn(ISADevice *dev) >> +{ >> + PCSpkState *s =3D 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); >=20 > Should pass dev as the first argument to isa_register_ioport. Otherwis= e > the resource won't be cleaned up during destruction. Oops, will fix. >=20 >> + >> + pcspk_state =3D s; >> + >> + return 0; >> +} >> + >> +static void pcspk_class_initfn(ObjectClass *klass, void *data) >> { >> - PCSpkState *s =3D&pcspk_state; >> + ISADeviceClass *ic =3D ISA_DEVICE_CLASS(klass); >> + ic->init =3D pcspk_initfn; >> +} >> >> - s->pit =3D 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 =3D { >> + .name =3D "isa-pcspk", >> + .size =3D sizeof(PCSpkState), >> + .no_user =3D 1, >> + .props =3D (Property[]) { >> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1), >> + DEFINE_PROP_PTR("pit", PCSpkState, pit), >=20 > 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? 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. Jan --------------enig3EFB9BA385D478AA68376E5F Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk8oZGMACgkQitSsb3rl5xRFKgCdFtvJ6YiBrBHpzW7CBxWHJdUS cksAoJ6Jq9XY62e/SqL0OoBem3/B92NM =8g84 -----END PGP SIGNATURE----- --------------enig3EFB9BA385D478AA68376E5F--