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

[BALATON Zoltan]

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.

> > +/* 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