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

[Jan Kiszka]

On 2012-01-31 23:40, Anthony Liguori wrote:
> 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<address@hidden>
>>>
>>> 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.

The current pattern requested by some reviewers used to be the one I
applied here. I dislike it as well when the device can't be seriously
configured out. But I can switch over, no problem.

> 
> And BTW, there is strict type checking now, which makes it already an
> improvement over property pointers.

OK, for my slow understanding: I use qdev_property_add_link in the
device init function to create the property, letting it point to a state
field. But what should I call from the outside to actually set its
value? And what C type does this value have? A DeviceState * or a char *
(path)?

Jan

signature.asc
Description: OpenPGP digital signature