Changeset 30992 in vbox

Timestamp:

Jul 22, 2010 1:38:57 PM (14 years ago)

Author:

vboxsync

Message:

HDA: A few more review comments.

Location:

trunk/src/VBox/Devices/Audio

Files:

• DevCodec.cpp (modified) (4 diffs)
• DevIchIntelHDA.cpp (modified) (4 diffs)

trunk/src/VBox/Devices/Audio/DevCodec.cpp

@@ -217 +217 @@
         case 0:
             pNode->root.node.name = "Root";
+            //** @todo r=michaln: I fear the use of RT_MAKE_U32_FROM_U8() here makes the
+            // code much harder to read, not easier.
             pNode->node.au32F00_param[0] = RT_MAKE_U32_FROM_U8(0x80, 0x76, 0x84, 0x83); /* VendorID = STAC9220/ DevId = 0x7680 */
             pNode->node.au32F00_param[2] = RT_MAKE_U32_FROM_U8(0x1, 0x31, 0x10, 0x00); /* rev id */
@@ -223 +225 @@
         case 1:
             pNode->afg.node.name = "AFG";
+            //** @todo r=michaln: Are the comments right? Looks like copy & paste.
             pNode->node.au32F00_param[4] = RT_MAKE_U32_FROM_U8(0x1a, 0x00, 0x02, 0x00); /* node info (start node: 1, start id = 1) */
             pNode->node.au32F00_param[5] = RT_MAKE_U32_FROM_U8(0x1, 0x01, 0x00, 0x0); /* node info (start node: 1, start id = 1) */
@@ -388 +391 @@
             pNode->node.name = "VolumeKnob";
             pNode->node.au32F00_param[0x9] = (0x6 << 20);
+            //** @todo r=michaln: The next assignment is out of array bounds (0x13 items in array);
+            //  the array needs to be bigger.
             pNode->node.au32F00_param[0x13] = RT_BIT(7)| 0x7F;
             pNode->node.au32F00_param[0xe] = 0x4;
@@ -577 +582 @@
     pState->cVerbs = STAC9220_VERB_SIZE;
     pState->pfnLookup = codecLookup;
+    //** @todo r=michaln: Where is this memory freed?
     pState->pNodes = (PCODECNODE)RTMemAllocZ(sizeof(CODECNODE) * STAC9220_NODE_COUNT);
     uint8_t i;

trunk/src/VBox/Devices/Audio/DevIchIntelHDA.cpp

@@ -91 +91 @@
 #define INTCTL_SALL(pState) (INTCTL((pState)) & 0xFF)
 
+/* Note: The HDA specification defines a SSYNC register at offset 0x38. The
+ * ICH6/ICH9 datahseet defines SSYNC at offset 0x34. The Linux HDA driver matches
+ * the datasheet.
+ */
 #define ICH6_HDA_REG_SSYNC  12 /* 0x34 */
 #define SSYNC(pState) (HDA_REG((pState), SSYNC))
@@ -424 +428 @@
 } s_ichIntelHDRegMap[] =
 {
-    //** @todo r=michaln: Please align the table properly for reasier readability.
     /* offset  size     read mask   write mask         read callback         write callback         abbrev      full name                     */
     /*-------  -------  ----------  ----------  -----------------------  ------------------------ ----------    ------------------------------*/
@@ -438 +441 @@
     { 0x00020, 0x00004, 0xC00000FF, 0xC00000FF, hdaRegReadU32          , hdaRegWriteU32          , "INTCTL"    , "Interrupt Control" },
     { 0x00024, 0x00004, 0xC00000FF, 0x400000FF, hdaRegReadINTSTS       , hdaRegWriteINTSTS       , "INTSTS"    , "Interrupt Status" },
+    //** @todo r=michaln: Are guests really not reading the WALCLK register at all?
     { 0x00030, 0x00004, 0xFFFFFFFF, 0x00000000, hdaRegReadUnimplemented, hdaRegWriteUnimplemented, "WALCLK"    , "Wall Clock Counter" },
+    //** @todo r=michaln: Doesn't the SSYNC register need to actually stop the stream(s)?
     { 0x00034, 0x00004, 0x000000FF, 0x000000FF, hdaRegReadU32          , hdaRegWriteU32          , "SSYNC"     , "Stream Synchronization" },
     { 0x00040, 0x00004, 0xFFFFFF80, 0xFFFFFF80, hdaRegReadU32          , hdaRegWriteBase         , "CORBLBASE" , "CORB Lower Base Address" },
@@ -1321 +1326 @@
     pThis->hda.cbCorbBuf = CORBSIZE(&pThis->hda) * sizeof(uint32_t);
 
+    //** @todo r=michaln: Where are the Corb/RirbBuf allocations freed?
     if (pThis->hda.pu32CorbBuf)
         memset(pThis->hda.pu32CorbBuf, 0, pThis->hda.cbCorbBuf);