Hi,
On 13/09/2024 16:17, Andy Shevchenko wrote:
> The PCI vendor ID for Cadence is defined in pci_ids.h. Use it.
> While at it, move to PCI_DEVICE() macro and usual pattern for
> PCI class and device IDs.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/cdns3/cdns3-pci-wrap.c | 5 ++---
> drivers/usb/cdns3/cdnsp-pci.c | 29 +++++++++++++++--------------
> 2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
> index 1f6320d98a76..591d149de8f3 100644
> --- a/drivers/usb/cdns3/cdns3-pci-wrap.c
> +++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
> @@ -37,8 +37,7 @@ struct cdns3_wrap {
> #define PCI_DRIVER_NAME "cdns3-pci-usbss"
> #define PLAT_DRIVER_NAME "cdns-usb3"
>
> -#define CDNS_VENDOR_ID 0x17cd
> -#define CDNS_DEVICE_ID 0x0100
> +#define PCI_DEVICE_ID_CDNS_USB3 0x0100
Why do we need to change this? You did not explain in commit log.
I would call this PCI_DEVICE_ID_CDNS_USBSS3. Also see later why to differentiate with USBSSP.
>
> static struct pci_dev *cdns3_get_second_fun(struct pci_dev *pdev)
> {
> @@ -190,7 +189,7 @@ static void cdns3_pci_remove(struct pci_dev *pdev)
> }
>
> static const struct pci_device_id cdns3_pci_ids[] = {
> - { PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
> + { PCI_VDEVICE(CDNS, PCI_DEVICE_ID_CDNS_USB3) },
For better readability I still prefer
PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USBSS3)
> { 0, }
> };
>
> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 225540fc81ba..2d05368a6745 100644
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c
> @@ -28,10 +28,11 @@
> #define PCI_DRIVER_NAME "cdns-pci-usbssp"
> #define PLAT_DRIVER_NAME "cdns-usbssp"
>
> -#define CDNS_VENDOR_ID 0x17cd
> -#define CDNS_DEVICE_ID 0x0200
> -#define CDNS_DRD_ID 0x0100
> -#define CDNS_DRD_IF (PCI_CLASS_SERIAL_USB << 8 | 0x80)
> +#define PCI_DEVICE_ID_CDNS_USB3 0x0100
This is an entirely different card who's device ID should be 0x200?
Also I don't think this card supports USB3 so it is a wrong name choice.
I would call this PCI_DEVICE_ID_CDNS_USBSSP 0x200 to match with PCI driver name.
> +#define PCI_DEVICE_ID_CDNS_UDC 0x0200
UDC is used for Peripheral controller only. Is that really the case here?
originally it was called DRD.
So how about?
PCI_DEVICE_ID_CDNS_DRD 0x0100
> +
> +#define PCI_CLASS_SERIAL_USB_CDNS_USB3 (PCI_CLASS_SERIAL_USB << 8 | 0x80)
> +#define PCI_CLASS_SERIAL_USB_CDNS_UDC PCI_CLASS_SERIAL_USB_DEVICE
>
> static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
> {
> @@ -40,10 +41,10 @@ static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
> * Platform has two function. The fist keeps resources for
> * Host/Device while the secon keeps resources for DRD/OTG.
> */
> - if (pdev->device == CDNS_DEVICE_ID)
> - return pci_get_device(pdev->vendor, CDNS_DRD_ID, NULL);
> - else if (pdev->device == CDNS_DRD_ID)
> - return pci_get_device(pdev->vendor, CDNS_DEVICE_ID, NULL);
> + if (pdev->device == PCI_DEVICE_ID_CDNS_UDC)
> + return pci_get_device(pdev->vendor, PCI_DEVICE_ID_CDNS_USB3, NULL);
> + if (pdev->device == PCI_DEVICE_ID_CDNS_USB3)
> + return pci_get_device(pdev->vendor, PCI_DEVICE_ID_CDNS_UDC, NULL);
>
> return NULL;
> }
> @@ -220,12 +221,12 @@ static const struct dev_pm_ops cdnsp_pci_pm_ops = {
> };
>
> static const struct pci_device_id cdnsp_pci_ids[] = {
> - { PCI_VENDOR_ID_CDNS, CDNS_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,
> - PCI_CLASS_SERIAL_USB_DEVICE, PCI_ANY_ID },
> - { PCI_VENDOR_ID_CDNS, CDNS_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,
> - CDNS_DRD_IF, PCI_ANY_ID },
> - { PCI_VENDOR_ID_CDNS, CDNS_DRD_ID, PCI_ANY_ID, PCI_ANY_ID,
> - CDNS_DRD_IF, PCI_ANY_ID },
> + { PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_UDC),
> + .class = PCI_CLASS_SERIAL_USB_CDNS_UDC },
> + { PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_UDC),
> + .class = PCI_CLASS_SERIAL_USB_CDNS_USB3 },
> + { PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USB3),
> + .class = PCI_CLASS_SERIAL_USB_CDNS_USB3 },
> { 0, }
> };
>
--
cheers,
-roger
