Re: [PATCH v1 1/1] sub: cdns3: Use predefined PCI vendor ID constant

[Roger Quadros <rogerq@kernel.org>]

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@linux.intel.com>
> ---
>  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