Revision is no longer valuable as a stand-alone type, because we only
ever want the full information that Spec provides. Therefore, enhance
Spec's Display, and remove Revision entirely.

This also simplifies the dev_info!() code for printing banners such as:

    NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs | 50 ++++++++++++++----------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 9d182bffe8b4..0ba1cdc42ba3 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -129,31 +129,11 @@ fn try_from(value: u8) -> Result<Self> {
     }
 }
 
-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,
 }
 
 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
+        )
+    }
+}
+
 /// Structure holding the resources required to operate the GPU.
 #[pin_data]
 pub(crate) struct Gpu {
@@ -193,13 +187,7 @@ pub(crate) fn new<'a>(
     ) -> impl PinInit<Self, Error> + 'a {
         try_pin_init!(Self {
             spec: Spec::new(bar).inspect(|spec| {
-                dev_info!(
-                    pdev.as_ref(),
-                    "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
-                    spec.chipset,
-                    spec.chipset.arch(),
-                    spec.revision
-                );
+                dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);
             })?,
 
             // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
-- 
2.51.1

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?

>  
>  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,
	)

> +    }
> +}
> +
>  /// Structure holding the resources required to operate the GPU.
>  #[pin_data]
>  pub(crate) struct Gpu {
> @@ -193,13 +187,7 @@ pub(crate) fn new<'a>(
>      ) -> impl PinInit<Self, Error> + 'a {
>          try_pin_init!(Self {
>              spec: Spec::new(bar).inspect(|spec| {
> -                dev_info!(
> -                    pdev.as_ref(),
> -                    "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
> -                    spec.chipset,
> -                    spec.chipset.arch(),
> -                    spec.revision
> -                );
> +                dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);
>              })?,
>  
>              // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
> -- 
> 2.51.1

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