On Fri, Nov 08, 2024 at 12:17:22PM +0000, Shijith Thotton wrote:
> >> This patch introduces a PCI hotplug controller driver for the OCTEON
> >> PCIe device, a multi-function PCIe device where the first function acts
> >> as a hotplug controller. It is equipped with MSI-x interrupts to notify
> >> the host of hotplug events from the OCTEON firmware.
> >> +config HOTPLUG_PCI_OCTEONEP
> >> + bool "OCTEON PCI device Hotplug controller driver"
> >
> >s/Marvell PCI device/Cavium OCTEON PCI/ to match other entries.
>
> Cavium was acquired by Marvell, but we are still using the PCI
> vendor ID `PCI_VENDOR_ID_CAVIUM`. I hope using Marvell is
> acceptable; please let me know if this poses any issues.
Oops, sorry, this was my mistake. I meant to suggest "Marvell OCTEON
PCI Hotplug driver", but I messed up the editing.
> >> +#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
> >
> >Even though this is a private #define, I think something like
> >PCI_DEVICE_ID_CAVIUM_OCTEP_HP_CTLR would be nice because that's the
> >typical pattern of include/linux/pci_ids.h.
>
> The same reason mentioned above for not using the name CAVIUM.
> Is it okay to use `PCI_DEVICE_ID_MARVELL_OCTEP_HP_CTLR`?
In this case, I think you should use PCI_DEVICE_ID_CAVIUM... because
the PCI_VENDOR_ID_CAVIUM identifies the Device ID namespace, so the
#defines for the Vendor ID and the Device IDs in that namespace should
match.
> >> +static struct pci_device_id octep_hp_pci_map[] = {
> >> + { PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
> >OCTEP_DEVID_HP_CONTROLLER) },
> >> + { },
> >> +};
