Re: [PATCH v3 3/3] rust: pci: provide access to PCI Vendor values

[John Hubbard <jhubbard@nvidia.com>]

On 8/19/25 2:16 AM, Danilo Krummrich wrote:
> On Tue Aug 19, 2025 at 5:11 AM CEST, John Hubbard wrote:
>> +/// PCI vendor IDs.
>> +///
>> +/// Each entry contains the 16-bit PCI vendor ID as assigned by the PCI SIG.
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// # use kernel::{device::Core, pci::{self, Vendor}, prelude::*};
>> +/// fn probe_device(pdev: &pci::Device<Core>) -> Result<()> {
>> +///     // Validate vendor ID
>> +///     let vendor = Vendor::try_from(pdev.vendor_id() as u32)?;
> 
> Why not change vendor_id() to return a Vendor instance directly?

Yes, will do.

> 
>> +///     dev_info!(
>> +///         pdev.as_ref(),
>> +///         "Detected vendor ID: (0x{:04x})\n",
>> +///         vendor.as_u32()
>> +///     );
>> +///     Ok(())
>> +/// }
>> +/// ```
>> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> +#[repr(transparent)]
>> +pub struct Vendor(u32);
> 
> [ Vendor impl and lots of ids... ]
> 
> Same as for Class; probably better to move it to its own module.
> 
> We could also move both Class and Vendor into a single module, e.g. id.rs and
> keep the module prefix. This would have the advantage that we could have
> pci::id::Class, pci::id::Vendor and pci::id::Device (which, eventually, we want
> as well), without getting a name conflict with pci::Device.

id.rs does provide a nice naming situation, I'll do that.

Thanks for the reviews again!


thanks,
-- 
John Hubbard