On Thu, Dec 12, 2024 at 05:33:40PM +0100, Danilo Krummrich wrote: > +impl DeviceId { > + const PCI_ANY_ID: u32 = !0; > + > + /// PCI_DEVICE macro. > + pub const fn new(vendor: u32, device: u32) -> Self { > + Self(bindings::pci_device_id { > + vendor, > + device, > + subvendor: DeviceId::PCI_ANY_ID, > + subdevice: DeviceId::PCI_ANY_ID, > + class: 0, > + class_mask: 0, > + driver_data: 0, > + override_only: 0, > + }) > + } > + > + /// PCI_DEVICE_CLASS macro. > + pub const fn with_class(class: u32, class_mask: u32) -> Self { I know naming is hard, and I'm not going to object to this at all, but using "new()" and "with_class()" feels a bit odd and mis-matched. How about spelling it out, pci_device(), and pci_device_class()? Anyway, not a bit deal at all, let's see how this plays out with real drivers and we can always change it later. > +// Allow drivers R/O access to the fields of `pci_device_id`; should we prefer accessor functions > +// to void exposing C structure fields? Minor nit, do you mean "to avoid exposing..."? Other than these, this looks good! If I can get an ack from the PCI maintainer, I'll be glad to queue these up in my driver core tree... thanks, greg k-h