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.
Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
