Re: [PATCH v3] Implement the Screamer sound chip for the mac99 machine type

[Programmingkid]

> On Feb 16, 2020, at 4:59 PM, BALATON Zoltan <address@hidden> wrote:
> 
> On Sun, 16 Feb 2020, Howard Spoelstra wrote:
>> On Sun, Feb 16, 2020 at 5:32 PM John Arbuckle <address@hidden>
>> wrote:
>>> diff --git a/hw/audio/screamer.c b/hw/audio/screamer.c
>>> new file mode 100644
>>> index 0000000000..ad4aba12eb
>>> --- /dev/null
>>> +++ b/hw/audio/screamer.c
>>> @@ -0,0 +1,983 @@
>>> +/*
>>> + * File: Screamer.c
>>> + * Description: Implement the Screamer sound chip used in Apple
>>> Macintoshes.
>>> + * It works by filling a buffer, then playing the buffer.
>>> + */
> 
> Do you need a copyright and license header here? Especially if this is not 
> all your original work but based on previous code (don't know if it is just 
> saying in case as I know Mark also had some similar patches before but not 
> sure how are those related if at all). If this contains code from somewhere 
> else then license and author of that code may need to be included too.

That is a good question. According to this page https://wiki.qemu.org/License, 
files that don't have licensing information default under the GNU GPL v2. I'm 
fine with that.

> 
>>> +/* Called when the CPU writes to the memory addresses assigned to
>>> Screamer */
>>> +static void screamer_mmio_write(void *opaque, hwaddr addr, uint64_t
>>> raw_value,
>>> +                                unsigned size)
>>> +{
>>> +    DPRINTF("screamer_mmio_write() called - size: %d\n", size);
>>> +    ScreamerState *state = opaque;
>>> +    uint32_t value = raw_value & 0xffffffff;
>>> +    addr = addr >> 4;
>>> +
>>> +    switch (addr) {
>>> +    case SOUND_CONTROL_REG:
>>> +        set_sound_control_reg(state, value);
>>> +        break;
>>> +    case CODEC_CONTROL_REG:
>>> +        set_codec_control_reg(state, value);
>>> +        break;
>>> +    case CODEC_STATUS_REG:
>>> +        set_codec_status_reg(state, value);
>>> +        break;
>>> +    case CLIP_COUNT_REG:
>>> +        set_clip_count_reg(state, value);
>>> +        break;
>>> +    case BYTE_SWAP_REG:
>>> +        set_byte_swap_reg(state, value);
>>> +        break;
>>> +    case FRAME_COUNT_REG:
>>> +        set_frame_count_reg(state, value);
>>> +        break;
>>> +    default:
>>> +        DPRINTF("Unknown register write - addr:%llu\tvalue:%d\n", addr,
>>> value);
>>> +    }
>>> +}
>>> 
>>> Hi,
>> 
>> This patch will not compile without errors. Host is Fedora 31.
>> The compiler suggests changing lines 839, 842 and 878 in screamer.c so the
>> DPRINTF arguments use %lu instead of %llu.
>> With that fixed, compiling completes succesfully.
> 
> Replacing with %lu may fix 32bit build but would break 64bit one. Use 
> HWADDR_PRIx format string instead to print hwaddr but others will probably 
> tell to remove DPRINTFs alltogether when they are not needed any more and 
> replace the remaining few useful ones with traces if debugging is still 
> needed. I don't mind DPRINTFs that much at least until things are stable 
> enough but once the code is stable most DPRINTFs may not be needed any more.
> 
> I can't really review the actual patch because I don't know audio in QEMU.
> 
> Regards,
> BALATON Zoltan

Your HWADDR_PRIx suggestion was great. I am making a small patch to test out 
your suggestion.

Thank you.