Re: [PATCH v2 1/2] gpu: nova-core: merge the Revision type into the Spec type

[John Hubbard]

On 10/28/25 5:25 AM, Danilo Krummrich wrote:

On Tue Oct 28, 2025 at 3:39 AM CET, John Hubbard wrote:

-pub(crate) struct Revision {
-    major: u8,
-    minor: u8,
-}
-
-impl Revision {
-    fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
-        Self {
-            major: boot0.major_revision(),
-            minor: boot0.minor_revision(),
-        }
-    }
-}
-
-impl fmt::Display for Revision {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{:x}.{:x}", self.major, self.minor)
-    }
-}
-
-/// Structure holding the metadata of the GPU.
+/// Structure holding a basic description of the GPU: Architecture, Chipset 
and Revision.
  pub(crate) struct Spec {
      chipset: Chipset,
-    /// The revision of the chipset.
-    revision: Revision,
+    major_revision: u8,
+    minor_revision: u8,
  }

Why not keep the Revision type and its Display impl as well?

I just felt like it's not quite pulling its weight. But clearly
that feeling is not widely shared, so I'll put it back in. :)

impl Spec {

@@ -162,11 +142,25 @@ fn new(bar: &Bar0) -> Result<Spec> {

Ok(Self {

              chipset: boot0.chipset()?,
-            revision: Revision::from_boot0(boot0),
+            major_revision: boot0.major_revision(),
+            minor_revision: boot0.minor_revision(),
          })
      }
  }

+impl fmt::Display for Spec {

+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "Chipset: {}, Architecture: {:?}, Revision: {:x}.{:x}",
+            self.chipset,
+            self.chipset.arch(),
+            self.major_revision,
+            self.minor_revision
+        )

This could just be:

        write!(
            f,
            "Chipset: {}, Architecture: {:?}, Revision: {}",
            self.chipset,
            self.chipset.arch(),
            self.revision,
        )

Yes. That is nicer.

thanks,
John Hubbard