On Thu Nov 6, 2025 at 10:27 AM JST, John Hubbard wrote: > 1) Implement Display for Spec. This simplifies the dev_info!() code for > printing banners such as: > > NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1) > > 2) Decouple Revision from boot0. > > 3) Enhance Revision, which in turn simplifies Spec::new(). > > 4) Also, slightly enhance the comment about Spec, to be more precise.
A bullet-list in a patch description is a sure sign you will be asked to split things up. :) And in this case I think it makes all the more sense, since all these things taken separately ought to be simple, but having them in the same diff makes it confusing to review. Although it's mostly the `Display` implementation that at the very least should be its own patch, the rest can probably be kept together as it is related, and adding an intermediate patch would require temporary code to build Revision. The diff becomes much clearer once the impl blocks are moved where they should be (please see below). The comment update can be squashed together with the Revision/Spec patch. > > Cc: Alexandre Courbot <[email protected]> > Cc: Danilo Krummrich <[email protected]> > Cc: Timur Tabi <[email protected]> > Signed-off-by: John Hubbard <[email protected]> > --- > drivers/gpu/nova-core/gpu.rs | 45 +++++++++++++++++++---------------- > drivers/gpu/nova-core/regs.rs | 8 +++++++ > 2 files changed, 33 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs > index 9d182bffe8b4..8173cdcd8378 100644 > --- a/drivers/gpu/nova-core/gpu.rs > +++ b/drivers/gpu/nova-core/gpu.rs > @@ -130,16 +130,18 @@ fn try_from(value: u8) -> Result<Self> { > } > > pub(crate) struct Revision { > - major: u8, > - minor: u8, > + pub(crate) major: u8, > + pub(crate) minor: u8, > } > > -impl Revision { > - fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self { > - Self { > - major: boot0.major_revision(), > - minor: boot0.minor_revision(), > - } > +impl TryFrom<regs::NV_PMC_BOOT_0> for Spec { > + type Error = Error; > + > + fn try_from(boot0: regs::NV_PMC_BOOT_0) -> Result<Self> { > + Ok(Self { > + chipset: boot0.chipset()?, > + revision: boot0.revision(), > + }) This impl block for `Revision` gets replaced by a block for `Spec`, which is only declared later. Can you move it (and the one for BOOT_42 in the third patch) to the right place, next to the other impl blocks for `Spec`? > } > } > > @@ -149,7 +151,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > } > } > > -/// 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. > @@ -160,10 +162,19 @@ impl Spec { > fn new(bar: &Bar0) -> Result<Spec> { > let boot0 = regs::NV_PMC_BOOT_0::read(bar); > > - Ok(Self { > - chipset: boot0.chipset()?, > - revision: Revision::from_boot0(boot0), > - }) > + Spec::try_from(boot0) > + } > +} > + > +impl fmt::Display for Spec { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + write!( > + f, > + "Chipset: {}, Architecture: {:?}, Revision: {}", > + self.chipset, > + self.chipset.arch(), > + self.revision > + ) > } > } > > @@ -193,13 +204,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. > diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs > index 206dab2e1335..207b865335af 100644 > --- a/drivers/gpu/nova-core/regs.rs > +++ b/drivers/gpu/nova-core/regs.rs > @@ -41,6 +41,14 @@ pub(crate) fn chipset(self) -> Result<Chipset> { > }) > .and_then(Chipset::try_from) > } > + > + /// Returns the revision information of the chip. > + pub(crate) fn revision(self) -> crate::gpu::Revision { > + crate::gpu::Revision { nit: let's import `Revision`, or at least `gpu`, to align with what we do with the other structures.
