[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH v3] Implement the Screamer sound chip for the mac99 machine type
Date: Sun, 16 Feb 2020 22:59:43 +0100 (CET)
User-agent: Alpine 2.22 (BSF 395 2020-01-19)

On Sun, 16 Feb 2020, Howard Spoelstra wrote:
On Sun, Feb 16, 2020 at 5:32 PM John Arbuckle <address@hidden>
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
+ * 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
+                                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) {
+        set_sound_control_reg(state, value);
+        break;
+        set_codec_control_reg(state, value);
+        break;
+        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,
+    }


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]