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

John Hubbard
2025-10-28T17:26:06Z

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