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

[Programmingkid <programmingkidx@gmail.com>]

> On Feb 16, 2020, at 4:59 PM, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> 
> On Sun, 16 Feb 2020, Howard Spoelstra wrote:
>> On Sun, Feb 16, 2020 at 5:32 PM John Arbuckle <programmingkidx@gmail.com>
>> 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.