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